Skip to content

feat(tasks): add golden PR evals for coding agents - #107390

Open
pauldambra wants to merge 7 commits into
masterfrom
posthog/golden-pr-evals
Open

pauldambra wants to merge 7 commits into
masterfrom
posthog/golden-pr-evals

Conversation

@pauldambra

@pauldambra pauldambra commented Sep 27, 2026 •

Copy link
Copy Markdown
Member

Problem

  • Nobody at PostHog can measure how close a coding agent gets to one-shotting a real PostHog pull request, or compare models on that.
  • The existing eval harness scores the PostHog agent against a seeded Hedgebox project, which does not answer that question.
  • A set of PRs from 2023 and 2024, written and reviewed by people before coding agents were common, gives a golden baseline that only had human interaction.

Changes

  • A person can trigger the new Golden PR Evals workflow from the Actions tab, pick a runtime (claude or codex), a model, and PR numbers, and read a score table in the run summary.
  • Each PR runs as its own matrix job. The agent gets a fresh git repository with only the tree at the commit before the PR, and the PR title and description as its prompt. It cannot read the merged PR from git history, and GitHub credentials are dropped from its environment.
  • Three scores per PR: share of the golden PR's files the agent also changed, F1 over added lines, and a judge score from an Anthropic model that reads the task and both diffs. Snapshot files and images are ignored.
  • The same runner works on a devbox: python -m products.tasks.evals.golden_prs run --pr 25832. Results, diffs and agent logs land in a gitignored results directory.
  • The judge uses the Anthropic SDK when ANTHROPIC_API_KEY is set, and the signed-in claude CLI otherwise, so a devbox needs no API key.
  • A run whose agent fails (logged out, timed out, non-zero exit) reports the failure in the table instead of a 0 score that reads like a bad attempt.
  • The agent workspace keeps the files git archive drops as export-ignore, including every .gitignore, so the agent's build output stays out of the candidate diff.
  • The golden set holds 12 PRs, 3 each from the four requested authors, mixing fixes and features from 2023 and 2024. README.md shows how to add one.
  • Standalone by design: the runner imports only the Anthropic SDK and pydantic, so the workflow installs no repo dependencies. That is also why the dataclasses use stdlib @dataclass(frozen=True, kw_only=True, slots=True) rather than posthog.dataclasses.frozen, whose package imports Django on load.
flowchart LR
    D([workflow_dispatch]) --> P[plan: select PRs]
    P --> E1[evaluate PR a]
    P --> E2[evaluate PR b]
    E1 --> R[report: score table]
    E2 --> R
    E1 --> A[(diff, agent log, scores)]
    E2 --> A
    classDef phBlue fill:#1d4aff,stroke:#1d4aff,color:#fff;
    classDef phYellow fill:#f9bd2b,stroke:#f9bd2b,color:#000;
    classDef phGray fill:#e5e7eb,stroke:#c7ccd1,color:#000;
    class E1,E2 phBlue;
    class D,R phYellow;
    class A,P phGray;
Loading

Note

The deterministic scores are strict. A correct change written a different way scores low on them, so read them together with the judge score and its reasoning. The six runs show three gaps to fix next: line F1 ignores deleted lines, files hit punishes a different file layout, and one judge sample per case is not stable when the approach differs from the golden one. Running the golden PR's own tests against the candidate is a possible next step, but needs the full stack at a 2023 commit and is out of scope here.

How did you test this code?

  • test_golden_prs.py: parameterized tests for diff parsing (rename, snapshot and image exclusion), the overlap scores (identical, empty, wrong files, partial, extra file), prompt cleaning of PR template noise, the judge short-circuit on an empty diff, credential stripping, and the report table. They also check every golden set entry is complete and from the four authors.
  • Smoke test on a devbox: fetched the golden commit for PR 25832 into this shallow clone, built the isolated checkout, applied the golden diff as the candidate, and got 1.0 on every deterministic score; half of the diff scored 0.5 files hit.
  • Six full 12-case runs from a Mac with the signed-in claude and codex CLIs, judged by claude-opus-5 through the CLI fallback. Tables, one line per PR, and the scores that look wrong are in the first results comment (claude-opus-5, gpt-5.5) and the second (claude-opus-5-5, claude-fable-5-1, gpt-6-sol, gpt-6-astra).
  • Not run: the workflow dispatch. workflow_dispatch only lists a workflow that exists on master, so the first dispatch happens after merge.
  • Not run: the SDK judge path with a real API key. The CLI fallback carried every run above.
  • hogli lint:workflows, actionlint, hogli ci:plan, and hogli product:lint tasks pass locally.

Release status

  • No feature flag controls this change

Automatic notifications

  • Publish to changelog?

Docs update

None. The runner's README covers usage.

🤖 Agent context

Autonomy: Human-driven (agent-assisted)

Agent: Claude Code, Claude Fable 5.1

Skills invoked: /writing-evals, /authoring-ci-workflows, /writing-tests, /writing-dataclasses, /writing-code-comments, /writing-user-facing-copy, /writing-pr-descriptions, /claude-api.

Later sessions on a Mac added the agent failure reporting, the CLI judge fallback, forced a/ and b/ diff prefixes (a host diff.mnemonicPrefix made the scorer miss every file), and the export-ignore restore, then ran the six model comparisons in the comments.

Decisions: the runner is a standalone package rather than a suite in products/posthog_ai/eval_harness, because that harness boots the test database, live server, LLM gateway, MCP server and Temporal, none of which this eval uses. The isolated checkout uses git archive plus git plumbing (write-tree, commit-tree) rather than a worktree or git commit, so shared objects and host commit hooks stay out of the agent's way. No open PR covers this; gh pr list --search found nothing.


Created with PostHog Desktop

🤖 Generated with Claude Code

@pauldambra pauldambra self-assigned this Sep 27, 2026
@trunk-io

trunk-io Bot commented Sep 27, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

@posthog

posthog Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

🦔 PostHog Review reviewed this pull request

Found 0 must fix, 2 should fix, 0 consider.

Published 2 findings (view the review).

@github-actions github-actions Bot added the feature/desktop Feature Tag: Desktop label Sep 27, 2026
@github-actions

github-actions Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

🤖 CI report

🚨 Trunk lane — universal lane

This PR is assigned to the universal lane. It cannot merge in parallel with other PRs, so it can take longer to merge. Ask dev-ex if you think this is wrong.

✅ Duplication (Python) — clean

New Python code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

✅ Duplication (TypeScript) — clean

New TypeScript code duplication introduced by this branch. Fails at 70+ tokens in app code, or 150+ tokens when both copies live in test files. Advisory while the gate proves itself: extract a shared helper instead of copying.

⚠️ Comment density — 4% of added code lines are comments (28 of 758)

This section warns when comments are more than 3% of the code lines a PR adds, and alerts above 6%. Before agent-assisted PRs, the typical share was about 2%. Only full-line comments count. Docstrings, generated files, snapshots, migrations, and workflow files are left out.

Comments that restate the code, record how the change came about, or narrate the next line add noise for the next reader. Keep the comments that explain a reason the code cannot show, and remove the rest. See .agents/skills/writing-code-comments/SKILL.md for the house rules.

Files with the most added comment lines:

File Comment lines Added lines
products/tasks/evals/golden_prs/workspace.py 16 139
products/tasks/evals/golden_prs/scoring.py 5 136
products/tasks/evals/golden_prs/agents.py 4 93
products/tasks/evals/golden_prs/cases.py 2 42
products/tasks/evals/golden_prs/__main__.py 1 137

This check does not block merging. It updates on every push and clears when the share drops.

✅ Bundle size — no change

Uncompressed size of every built .js bundle, compared against the base branch.

Total: 68.88 MiB · no change

No file changed by more than 1000 B.

Posted automatically by build-bundle-size-report · uncompressed bytes from dist-report

✅ Eager graph — within budget

How much code each root ships on the eager path — downloaded and parsed before the surface is interactive. Measured from the esbuild output chunks (post-tree-shake, static imports only); lazy import() / React.lazy chunks are not counted.

Root Eager (shipped) Δ vs base Budget
entry (logged-out pages, app bootstrap)
src/index.tsx
1.57 MiB · 22 files no change █████████░ 85.5% of 1.84 MiB
logged-out boot: index + App + bootApp (preloaded by every page, including /login)
src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
3.51 MiB · 629 files no change █████████░ 87.2% of 4.03 MiB
authenticated shell (every logged-in page)
src/scenes/AuthenticatedShell.tsx
7.34 MiB · 2,339 files no change █████████░ 88.0% of 8.34 MiB

🟢 node_modules/monaco-editor/ stays out of src/index.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 [object Object] stays out of src/index.tsx
🟢 node_modules/monaco-editor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/layout/navigation-3000/navigationLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/scenes/dashboard/dashboardLogic.tsx stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/lemon-ui/LemonMarkdown/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/RichContentEditor/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/lib/components/CodeSnippet/ stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 src/taxonomy/core-filter-definitions-by-group.json stays out of src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
🟢 node_modules/monaco-editor/ stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/lib/components/ActivityLog/describers stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 src/scenes/session-recordings/player/sessionRecordingPlayerLogic.ts stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx
🟢 [object Object] stays out of src/scenes/AuthenticatedShell.tsx

Largest files eagerly shipped from src/index.tsx
Size File
126.8 KiB ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
24.6 KiB ../node_modules/.pnpm/buffer@6.0.3/node_modules/buffer/index.js
6.3 KiB ../node_modules/.pnpm/react@18.3.1/node_modules/react/cjs/react.production.min.js
4.5 KiB ../node_modules/.pnpm/@jspm+core@2.1.0/node_modules/@jspm/core/nodelibs/browser/process.js
3.9 KiB ../node_modules/.pnpm/scheduler@0.23.2/node_modules/scheduler/cjs/scheduler.production.min.js
1.4 KiB ../node_modules/.pnpm/base64-js@1.5.1/node_modules/base64-js/index.js
1.3 KiB src/index.tsx
1.3 KiB src/RootErrorBoundary.tsx
912 B ../node_modules/.pnpm/ieee754@1.2.1/node_modules/ieee754/index.js
854 B src/scenes/ChunkLoadErrorBoundary.tsx
Largest files eagerly shipped from src/index.tsx + src/scenes/App.tsx + src/scenes/bootApp.ts
Size File
301.8 KiB ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs
216.0 KiB ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js
126.8 KiB ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
100.5 KiB src/lib/api.ts
88.4 KiB src/products.tsx
69.4 KiB src/lib/lemon-ui/icons/icons.tsx
40.1 KiB src/lib/utils/eventUsageLogic.ts
38.7 KiB ../node_modules/.pnpm/@dnd-kit+core@6.0.8_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@dnd-kit/core/dist/core.esm.js
33.9 KiB ../node_modules/.pnpm/kea@4.0.0-pre.6_patch_hash=139b8d1f1304f9d9da452a9a1244c94ea679dbcb85687d8999563146879fb6f5_react@18.3.1/node_modules/kea/lib/index.cjs.js
28.4 KiB src/scenes/scenes.ts
Largest files eagerly shipped from src/scenes/AuthenticatedShell.tsx
Size File
301.8 KiB ../node_modules/.pnpm/posthog-js@1.434.14_@types+react@18.3.27_react@18.3.1/node_modules/posthog-js/dist/module.mjs
271.7 KiB src/taxonomy/core-filter-definitions-by-group.json
216.0 KiB ../node_modules/.pnpm/@posthog+icons@0.38.0_react-dom@18.3.1_react@18.3.1__react@18.3.1/node_modules/@posthog/icons/dist/posthog-icons.es.js
153.7 KiB ../node_modules/.pnpm/re2js@0.4.1/node_modules/re2js/build/index.esm.js
126.8 KiB ../node_modules/.pnpm/react-dom@18.3.1_react@18.3.1/node_modules/react-dom/cjs/react-dom.production.min.js
100.5 KiB src/lib/api.ts
98.5 KiB ../packages/quill/packages/quill/dist/index.js
93.3 KiB ../node_modules/.pnpm/prosemirror-view@1.40.1/node_modules/prosemirror-view/dist/index.js
90.6 KiB ../node_modules/.pnpm/@tiptap+core@3.20.6_@tiptap+pm@3.20.6/node_modules/@tiptap/core/dist/index.js
88.4 KiB src/products.tsx

Posted automatically by check-eager-graph · sizes are eager output bytes (shipped, post-tree-shake) from the esbuild metafile · part of #32479

✅ Toolbar bundle — eager 2.16 MiB within budget

What the toolbar ships to customer pages, measured from the esbuild output (minified, post-tree-shake). The eager set is the entry plus everything statically imported from it — fetched before any feature runs; deferred chunks load lazily. The eager guardrail is 5.72 MiB. Each output file must also stay below 10 MB, where CloudFront stops compressing it. The module boundary is enforced separately by check-toolbar-graph.

Metric Size Δ vs base Budget
Eager (shipped)
entry + static imports
2.16 MiB · 19 files no change ████░░░░░░ 37.7% of 5.72 MiB
Deferred (lazy) 2.10 MiB · 44 files no change n/a — loads on demand
Loader dist/toolbar.js 1.2 KiB no change █░░░░░░░░░ 6.0% of 19.5 KiB
Largest eagerly-shipped chunks
Size File
800.5 KiB dist/toolbar/toolbar-app-QUJ43CJ4.css
651.6 KiB dist/toolbar/chunk-chunk-ZPCK2O6G.js
259.4 KiB dist/toolbar/chunk-chunk-CV2VU6SQ.js
138.3 KiB dist/toolbar/chunk-chunk-DYPTRYMF.js
131.8 KiB dist/toolbar/chunk-chunk-FDH2IBXT.js
75.2 KiB dist/toolbar/toolbar-app-4HYNQ5KU.js
69.0 KiB dist/toolbar/chunk-chunk-TSAL54PB.js
35.6 KiB dist/toolbar/chunk-chunk-HOKNU4ZL.js
21.0 KiB dist/toolbar/chunk-chunk-Z5ELNJKM.js
6.8 KiB dist/toolbar/chunk-chunk-DV7IWQNF.js

Posted automatically by check-toolbar-size · sizes are toolbar output bytes (shipped, post-tree-shake) from the esbuild metafile

✅ Dist folder size — no change

Total size of the built frontend/dist folder (all assets), compared against the base branch.

Total: 946.25 MiB · no change

@coderabbitai

coderabbitai Bot commented Sep 27, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

Important

Review skipped

We couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting @coderabbitai full review.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds a Golden PR evaluation tool with a dataset of 12 pull requests. The CLI runs Claude or Codex in a temporary parent checkout, scores candidate diffs, and saves results and reports. A manual GitHub Actions workflow selects cases, runs evaluations, and publishes combined results. The change also adds usage documentation, test coverage, and the evaluation tests to the backend test command.

Priority: ⬇️ Low

Merge Risk: 🟡 Moderate · up to 67872

This is a manually triggered, internal evaluation tool, so production users are unaffected. However, the candidate-diff step can run an agent-configured command with the evaluator's API key and let the agent fake its own diff, and several scoring and reporting accuracy issues remain open. Resolve the diff sanitization before relying on these scores or running with secrets.

Security Architecture Review

Security architecture risk: 🟡 Moderate · up to 67872

The new evaluation workflow may expose credentials to commands influenced by evaluated code. Manual triggering and isolated runs limit the likely scope, but the credential boundary needs attention.

Retained concerns

  • High · security · inferred: Git diffing of an agent-modifiable repository restores credentials withheld during staging, allowing agent-controlled Git configuration to influence a credential-bearing subprocess. Failed and timed-out agent runs also reach this step.
Security review details

Security Blast Radius

  • inferred — Effective exposure is the credentials available to one evaluation process and its subsequent Git subprocesses, on a hosted case runner or local devbox. Fresh workspaces limit ordinary cross-case persistence, but do not remove exposure during the case.

Security Findings and Attack Paths

  • inferred — Both retained findings converge on the same path: agent-modifiable repository configuration can affect post-agent Git diffing, whose default environment restores credentials excluded from staging. This establishes a potential secret-exposure path, not evidence that exfiltration occurred.

Trust Boundaries and Controls

  • observed — Credential-free checkout and a fresh archive-based repository restrict direct access to persisted checkout credentials and the shared Git object store. The agent commands nevertheless explicitly bypass permission or sandbox restrictions; those checkout controls do not isolate the runner filesystem.

Resilience and Maintainability Implications

  • inferred — Cleanup limits persistence after normal completion or an exception, but cannot protect credentials used before cleanup: nonzero and timed-out runs still proceed through post-agent diff collection. A broader filesystem sandbox or runner policy was not established by the available source.

Hardening Proposals

  • proposed — Run every post-agent Git operation with a consistent secret-free environment and neutralize agent-controlled Git execution hooks during diff collection. Separate credential-bearing judging from the mutable workspace where feasible.
🚥 Pre-merge checks | ✅ 1
✅ Passed checks (1 passed)
Check name Status Explanation
Description check ✅ Passed The description follows the required structure and clearly documents the problem, changes, testing, release status, documentation status, agent context, workflow behavior, limitations, and follow-up w…
✨ Finishing Touches 💡 1
🛠️ Fix failing CI checks 💡
  • Commit to this branch
  • Create a new PR
📝 Generate docstrings
  • Commit to this branch
  • Create a new PR
🧪 Generate unit tests (beta)
  • Commit to this branch
  • Create a new PR

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 5

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (3)
products/tasks/evals/golden_prs/cases.py-34-37 (1)

34-37: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Materialize numbers before checking membership.

If a caller passes a generator, set(numbers) exhausts it. The return expression then selects no PRs, even when every number is valid. Convert numbers to a list once and use that list for both operations.

products/tasks/evals/golden_prs/scoring.py-60-60 (1)

60-60: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Distinguish added source lines from file headers.

A diff line such as +++counter represents added source text ++counter, but this condition discards it. Line F1 can therefore undercount valid additions. Exclude the actual +++ b/... header instead of every line with the +++ prefix.

.github/workflows/golden-pr-evals.yml-66-66 (1)

66-66: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Deduplicate PR numbers before creating the matrix.

If prs contains the same number twice, both jobs use the same golden-pr-${{ matrix.pr }} artifact name. upload-artifact does not allow two jobs to create an artifact with the same name, so one evaluation can fail after consuming agent time. Preserve the requested order while removing duplicates. (github.com)


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: 731ee24d-0807-4af4-9b9b-92a8d96472af

📥 Commits

Reviewing files that changed from the base of the PR and between 066a38f and 8ecc5d7.

📒 Files selected for processing (13)
  • .github/workflows/golden-pr-evals.yml
  • products/tasks/evals/__init__.py
  • products/tasks/evals/golden_prs/.gitignore
  • products/tasks/evals/golden_prs/README.md
  • products/tasks/evals/golden_prs/__init__.py
  • products/tasks/evals/golden_prs/__main__.py
  • products/tasks/evals/golden_prs/agents.py
  • products/tasks/evals/golden_prs/cases.py
  • products/tasks/evals/golden_prs/golden_prs.json
  • products/tasks/evals/golden_prs/scoring.py
  • products/tasks/evals/golden_prs/test_golden_prs.py
  • products/tasks/evals/golden_prs/workspace.py
  • products/tasks/package.json

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 5 remain after this review.

Comment thread .github/workflows/golden-pr-evals.yml
Comment thread products/tasks/evals/golden_prs/agents.py
Comment on lines +113 to +114
f"<golden_diff>\n{_bounded(golden)}\n</golden_diff>\n\n"
f"<candidate_diff>\n{_bounded(candidate)}\n</candidate_diff>"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Remove artifact sections before bounding judge diffs.

score_diffs excludes artifacts, but judge sends them to _bounded. If a snapshot section fills the first 120,000 characters, the judge never receives later source changes and can score an otherwise relevant attempt incorrectly. Filter artifact sections from both diffs before truncation.

Comment thread products/tasks/evals/golden_prs/workspace.py
Comment thread products/tasks/evals/golden_prs/workspace.py Outdated
@posthog

posthog Bot commented Sep 27, 2026

Copy link
Copy Markdown
Contributor

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review
PostHog Review alpha 🦔 If you find any issues helpful - please reply "valid", "invalid", etc., for evaluation purposes 🙏

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

PostHog Review

Found 4 must fix, 10 should fix, 2 consider.

Comment thread products/tasks/evals/golden_prs/workspace.py
Comment on lines +88 to +91
def _bounded(diff: str) -> str:
if len(diff) <= MAX_DIFF_CHARS_FOR_JUDGE:
return diff
return diff[:MAX_DIFF_CHARS_FOR_JUDGE] + "\n[diff truncated for the judge]\n"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

Preserve relevant changes when bounding judge input

should_fix bug

Issue description

_bounded keeps only the first 120,000 characters. If important changes occur later in a large diff, the judge cannot consider them even though it must score the full behavior. This can produce misleading scores for large PRs or candidate diffs.

Why we think it's a valid issue
  • Checked: Traced _bounded in products/tasks/evals/golden_prs/scoring.py and its calls from judge(). Checked how evaluate() uses the verdict and what the report displays.
  • Found: products/tasks/evals/golden_prs/scoring.py:88-91 returns only the first 120,000 characters and adds a truncation marker. products/tasks/evals/golden_prs/scoring.py:113-114 applies this independently to both diffs. The judge prompt at products/tasks/evals/golden_prs/scoring.py:22-29 asks for a behavioral score, but does not tell the judge to treat omitted content as unknown. products/tasks/evals/golden_prs/__main__.py:51-65 stores that score, which the report presents.
  • Impact: Large diffs can contain relevant behavior after the cutoff, and no code ensures the retained prefix represents the full change. The judge can therefore assign a misleading score based on incomplete evidence. This meets the correctness bar for the eval's reported judge score.
Suggested fix

Bound diffs in a way that represents changes across the full diff, such as selecting hunks across files and listing omitted files. Tell the judge what was omitted so it does not treat the partial diff as complete.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/scoring.py#L88-91

<issue_description>
`_bounded` keeps only the first 120,000 characters. If important changes occur later in a large diff, the judge cannot consider them even though it must score the full behavior. This can produce misleading scores for large PRs or candidate diffs.
</issue_description>

<issue_validation>
- **Checked:** Traced `_bounded` in `products/tasks/evals/golden_prs/scoring.py` and its calls from `judge()`. Checked how `evaluate()` uses the verdict and what the report displays.
- **Found:** `products/tasks/evals/golden_prs/scoring.py:88-91` returns only the first 120,000 characters and adds a truncation marker. `products/tasks/evals/golden_prs/scoring.py:113-114` applies this independently to both diffs. The judge prompt at `products/tasks/evals/golden_prs/scoring.py:22-29` asks for a behavioral score, but does not tell the judge to treat omitted content as unknown. `products/tasks/evals/golden_prs/__main__.py:51-65` stores that score, which the report presents.
- **Impact:** Large diffs can contain relevant behavior after the cutoff, and no code ensures the retained prefix represents the full change. The judge can therefore assign a misleading score based on incomplete evidence. This meets the correctness bar for the eval's reported judge score.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Bound diffs in a way that represents changes across the full diff, such as selecting hunks across files and listing omitted files. Tell the judge what was omitted so it does not treat the partial diff as complete.
</potential_solution>

Comment thread products/tasks/evals/golden_prs/workspace.py Outdated
Comment thread .github/workflows/golden-pr-evals.yml
if not candidate.strip():
return Verdict(score=0.0, reasoning="The agent changed no files.")
client = client or anthropic.Anthropic()
response = client.messages.parse(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

Preserve eval results when the judge request fails

should_fix performance

Issue description

If the Anthropic request fails after its retries, judge() raises and aborts the evaluation. The runner writes the candidate diff, agent log, and deterministic scores only after judge() returns, so a transient API failure loses the completed agent run and prevents later PRs in the batch from running.

Why we think it's a valid issue
  • Checked: Traced the judge call through evaluate() and the CLI loop in products/tasks/evals/golden_prs/__main__.py. Checked the GitHub workflow’s matrix failure behavior and artifact upload condition.
  • Found: products/tasks/evals/golden_prs/scoring.py:104 makes the API request without handling request errors. products/tasks/evals/golden_prs/__main__.py:51 calls judge() before returning the case result; products/tasks/evals/golden_prs/__main__.py:136-137 writes the diff, log, and scores only after evaluate() returns. A CLI batch therefore stops at a failed judge request without saving that case. In the workflow, fail-fast: false allows other matrix jobs to continue, but the failed case still has no result files for the unconditional artifact upload to preserve.
  • Impact: A judge request failure after SDK retries can discard a completed agent run and its deterministic scores, requiring an expensive rerun to recover them. The later-PR impact applies to CLI batches, not the GitHub matrix.
Suggested fix

Handle judge failures as a separately recorded outcome. Persist the candidate diff, agent log, and deterministic scores even when no verdict is available, and report the judge error without treating it as a genuine zero score.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/scoring.py#L104

<issue_description>
If the Anthropic request fails after its retries, `judge()` raises and aborts the evaluation. The runner writes the candidate diff, agent log, and deterministic scores only after `judge()` returns, so a transient API failure loses the completed agent run and prevents later PRs in the batch from running.
</issue_description>

<issue_validation>
- **Checked:** Traced the judge call through `evaluate()` and the CLI loop in `products/tasks/evals/golden_prs/__main__.py`. Checked the GitHub workflow’s matrix failure behavior and artifact upload condition.
- **Found:** `products/tasks/evals/golden_prs/scoring.py:104` makes the API request without handling request errors. `products/tasks/evals/golden_prs/__main__.py:51` calls `judge()` before returning the case result; `products/tasks/evals/golden_prs/__main__.py:136-137` writes the diff, log, and scores only after `evaluate()` returns. A CLI batch therefore stops at a failed judge request without saving that case. In the workflow, `fail-fast: false` allows other matrix jobs to continue, but the failed case still has no result files for the unconditional artifact upload to preserve.
- **Impact:** A judge request failure after SDK retries can discard a completed agent run and its deterministic scores, requiring an expensive rerun to recover them. The later-PR impact applies to CLI batches, not the GitHub matrix.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Handle judge failures as a separately recorded outcome. Persist the candidate diff, agent log, and deterministic scores even when no verdict is available, and report the judge error without treating it as a genuine zero score.
</potential_solution>

Comment on lines +59 to +67
completed = subprocess.run(
agent_command(runtime, model),
cwd=workdir,
env=agent_environment(os.environ),
input=prompt,
capture_output=True,
text=True,
timeout=timeout_seconds,
check=False,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

[should_fix] Terminate agent child processes on timeout

should_fix performance

Issue description

When the timeout expires, subprocess.run terminates the CLI process but does not terminate its child processes. Agent CLIs can start shell commands and other tools, so those processes may keep using resources or modifying the checkout after the run times out. This can also make the collected candidate diff unreliable.

Why we think it's a valid issue
  • Checked: Reviewed the CLI launch flags and the timeout handling in run_agent().
  • Found: run_agent() starts the CLI with subprocess.run(..., timeout=timeout_seconds) at products/tasks/evals/golden_prs/agents.py:59-67. The CLI runs with approval and sandbox checks bypassed at products/tasks/evals/golden_prs/agents.py:30-33, so it can launch shell commands and other child processes. The timeout handler records the run as timed out but does not manage a process group at products/tasks/evals/golden_prs/agents.py:67-67.
  • Impact: If a child process survives termination of the CLI, it can keep consuming resources or modify the checkout while evaluate() collects the candidate diff. This can leave local runs with lingering processes and make results unreliable.
Suggested fix

Run the CLI in a new process session and, on timeout, terminate its process group with a graceful signal followed by a forced kill if needed. This keeps the timeout bounded for the full agent process tree.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/agents.py#L59-67

<issue_description>
When the timeout expires, `subprocess.run` terminates the CLI process but does not terminate its child processes. Agent CLIs can start shell commands and other tools, so those processes may keep using resources or modifying the checkout after the run times out. This can also make the collected candidate diff unreliable.
</issue_description>

<issue_validation>
- **Checked:** Reviewed the CLI launch flags and the timeout handling in `run_agent()`.
- **Found:** `run_agent()` starts the CLI with `subprocess.run(..., timeout=timeout_seconds)` at `products/tasks/evals/golden_prs/agents.py:59-67`. The CLI runs with approval and sandbox checks bypassed at `products/tasks/evals/golden_prs/agents.py:30-33`, so it can launch shell commands and other child processes. The timeout handler records the run as timed out but does not manage a process group at `products/tasks/evals/golden_prs/agents.py:67-67`.
- **Impact:** If a child process survives termination of the CLI, it can keep consuming resources or modify the checkout while `evaluate()` collects the candidate diff. This can leave local runs with lingering processes and make results unreliable.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Run the CLI in a new process session and, on timeout, terminate its process group with a graceful signal followed by a forced kill if needed. This keeps the timeout bounded for the full agent process tree.
</potential_solution>

Comment on lines +59 to +65
completed = subprocess.run(
agent_command(runtime, model),
cwd=workdir,
env=agent_environment(os.environ),
input=prompt,
capture_output=True,
text=True,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

[should_fix] Bound captured agent output

should_fix performance

Issue description

capture_output=True buffers all stdout and stderr in memory until the agent exits. A run can last 30 minutes, and Codex emits a JSON event stream, so a verbose run can consume substantial memory and fail the evaluation worker before results are written.

Why we think it's a valid issue
  • Checked: Traced how run_agent() captures output and how evaluate() stores it.
  • Found: subprocess.run() captures all stdout and stderr in memory at products/tasks/evals/golden_prs/agents.py:59-68, and AgentRun retains both strings at products/tasks/evals/golden_prs/agents.py:73-81. evaluate() combines them into the agent log at products/tasks/evals/golden_prs/__main__.py:71, then writes the log only after the run at products/tasks/evals/golden_prs/__main__.py:74-78. There is no output-size limit or incremental write.
  • Impact: If an agent command produces large output during a run, the runner holds that output in memory until the run finishes and can exhaust available memory before it writes the results.
Suggested fix

Stream output to a log file or process it incrementally, and keep only a bounded amount in memory. Preserve the Claude usage fields needed for scoring while avoiding full-output buffering.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/agents.py#L59-65

<issue_description>
`capture_output=True` buffers all stdout and stderr in memory until the agent exits. A run can last 30 minutes, and Codex emits a JSON event stream, so a verbose run can consume substantial memory and fail the evaluation worker before results are written.
</issue_description>

<issue_validation>
- **Checked:** Traced how `run_agent()` captures output and how `evaluate()` stores it.
- **Found:** `subprocess.run()` captures all stdout and stderr in memory at `products/tasks/evals/golden_prs/agents.py:59-68`, and `AgentRun` retains both strings at `products/tasks/evals/golden_prs/agents.py:73-81`. `evaluate()` combines them into the agent log at `products/tasks/evals/golden_prs/__main__.py:71`, then writes the log only after the run at `products/tasks/evals/golden_prs/__main__.py:74-78`. There is no output-size limit or incremental write.
- **Impact:** If an agent command produces large output during a run, the runner holds that output in memory until the run finishes and can exhaust available memory before it writes the results.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Stream output to a log file or process it incrementally, and keep only a bounded amount in memory. Preserve the Claude usage fields needed for scoring while avoiding full-output buffering.
</potential_solution>

Comment thread .github/workflows/golden-pr-evals.yml
Comment thread products/tasks/evals/golden_prs/cases.py
Comment thread products/tasks/evals/golden_prs/agents.py Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
products/tasks/evals/golden_prs/__main__.py-43-45 (1)

43-45: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win

Persist the agent failure reason for partial diffs.

When the agent exits nonzero after producing a candidate diff, keep judging the diff, but also store agent_failure(run) in a nullable agent_error field. The JSON result currently stores only exit_code, and report shows only judge reasoning. The .agent.log retains raw output, but the structured result and report do not show why the run failed.

Suggested fix
@@
     duration_seconds: float
     exit_code: int
+    agent_error: str | None
     timed_out: bool
@@
-    verdict = verdict_for(run, prompt, candidate, golden, judge_model)
+    failure = agent_failure(run)
+    verdict = verdict_for(run, prompt, candidate, golden, judge_model)
@@
         duration_seconds=run.duration_seconds,
         exit_code=run.exit_code,
+        agent_error=failure,
         timed_out=run.timed_out,
@@
-    reasoning = "\n".join(f"- **#{r['pr']}** ({r['judge_score']:.2f}): {r['judge_reasoning']}" for r in results)
+    reasoning = "\n".join(
+        f"- **#{r['pr']}** ({r['judge_score']:.2f}): {r['judge_reasoning']}"
+        + (f" Agent error: {r['agent_error']}" if r.get("agent_error") else "")
+        for r in results
+    )

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: f4a497ac-3b38-493a-9d2c-c7d29aa25121

📥 Commits

Reviewing files that changed from the base of the PR and between 8ecc5d7 and 0be288d.

📒 Files selected for processing (3)
  • products/tasks/evals/golden_prs/__main__.py
  • products/tasks/evals/golden_prs/agents.py
  • products/tasks/evals/golden_prs/test_golden_prs.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 10 remain after this review.

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

PostHog Review

Found 2 must fix, 4 should fix, 4 consider.

Comment on lines +53 to +56
def agent_usage(run: AgentRun) -> dict[str, float | int]:
"""Cost and turn count as the Claude CLI reports them; Codex emits an event stream we do not parse."""
report = _claude_report(run)
return {key: report[key] for key in ("total_cost_usd", "num_turns") if key in report}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

[should_fix] Do not report unavailable Codex cost as zero

should_fix bug

Issue description

agent_usage returns no usage data for Codex. The report then displays a missing cost as $0.00, which makes an unknown cost look measured and can mislead users comparing runtimes.

Why we think it's a valid issue
  • Checked: Traced agent_usage through evaluate into report, and checked the report test for missing usage.
  • Found: products/tasks/evals/golden_prs/agents.py:44-56 returns no usage for Codex. products/tasks/evals/golden_prs/__main__.py:74 stores that result, and products/tasks/evals/golden_prs/__main__.py:100 formats a missing total_cost_usd as 0.00. products/tasks/evals/golden_prs/test_golden_prs.py:188 confirms the zero-cost display for empty usage.
  • Impact: The report presents an unmeasured cost as a measured zero, making cost comparisons between runtimes misleading.
Suggested fix

Parse Codex usage data when available, or display an unavailable marker instead of defaulting a missing cost to zero.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/agents.py#L53-56

<issue_description>
`agent_usage` returns no usage data for Codex. The report then displays a missing cost as `$0.00`, which makes an unknown cost look measured and can mislead users comparing runtimes.
</issue_description>

<issue_validation>
- **Checked:** Traced `agent_usage` through `evaluate` into `report`, and checked the report test for missing usage.
- **Found:** `products/tasks/evals/golden_prs/agents.py:44-56` returns no usage for Codex. `products/tasks/evals/golden_prs/__main__.py:74` stores that result, and `products/tasks/evals/golden_prs/__main__.py:100` formats a missing `total_cost_usd` as `0.00`. `products/tasks/evals/golden_prs/test_golden_prs.py:188` confirms the zero-cost display for empty usage.
- **Impact:** The report presents an unmeasured cost as a measured zero, making cost comparisons between runtimes misleading.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Parse Codex usage data when available, or display an unavailable marker instead of defaulting a missing cost to zero.
</potential_solution>

if header:
counting = not is_artifact(header.group(2))
elif counting and line.startswith("+") and not line.startswith("+++"):
content = line[1:].strip()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

[should_fix] Preserve indentation in added-line scoring

should_fix bug

Issue description

strip() removes leading indentation before the line is scored. In Python, indentation can change behavior or make code invalid, but lines such as return value and return value then count as an exact match. This can inflate the deterministic line F1 score for an incorrect candidate.

Why we think it's a valid issue
  • Checked: Traced added_lines into _f1 and the displayed added_line_f1 score. Checked the existing test for added-line normalization.
  • Found: products/tasks/evals/golden_prs/scoring.py:61 strips leading and trailing whitespace before storing each added line. products/tasks/evals/golden_prs/scoring.py:67-73 then counts normalized lines as exact overlap. products/tasks/evals/golden_prs/__main__.py:98 displays that score. The test at products/tasks/evals/golden_prs/test_golden_prs.py:84-86 covers blank-line skipping but does not check indentation.
  • Impact: Python indentation can change syntax and control flow. The F1 score can count differently indented lines as identical, so it can overstate line overlap for a materially different candidate.
Suggested fix

Preserve leading whitespace when extracting added lines. Skip blank lines with a separate content.strip() check, and only remove trailing whitespace if that is the intended normalization.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/scoring.py#L61

<issue_description>
`strip()` removes leading indentation before the line is scored. In Python, indentation can change behavior or make code invalid, but lines such as `    return value` and `return value` then count as an exact match. This can inflate the deterministic line F1 score for an incorrect candidate.
</issue_description>

<issue_validation>
- **Checked:** Traced `added_lines` into `_f1` and the displayed `added_line_f1` score. Checked the existing test for added-line normalization.
- **Found:** `products/tasks/evals/golden_prs/scoring.py:61` strips leading and trailing whitespace before storing each added line. `products/tasks/evals/golden_prs/scoring.py:67-73` then counts normalized lines as exact overlap. `products/tasks/evals/golden_prs/__main__.py:98` displays that score. The test at `products/tasks/evals/golden_prs/test_golden_prs.py:84-86` covers blank-line skipping but does not check indentation.
- **Impact:** Python indentation can change syntax and control flow. The F1 score can count differently indented lines as identical, so it can overstate line overlap for a materially different candidate.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Preserve leading whitespace when extracting added lines. Skip blank lines with a separate `content.strip()` check, and only remove trailing whitespace if that is the intended normalization.
</potential_solution>

Comment thread products/tasks/evals/golden_prs/__main__.py
with checkout_parent(repo, pr) as workdir:
run = run_agent(runtime, model, prompt, workdir, timeout_seconds)
candidate = candidate_diff(workdir)
verdict = verdict_for(run, prompt, candidate, golden, judge_model)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

[should_fix] Preserve agent artifacts when judging fails

should_fix bug

Issue description

If the Anthropic request in judge() fails after its retries, evaluate() raises before returning the candidate diff and agent log. The caller therefore never reaches write_result(), and the workflow's always-run artifact upload has no completed agent output to preserve. This discards work already spent running the agent and prevents retrying only the judge.

Why we think it's a valid issue
  • Checked: Traced evaluate(), judge(), the result-writing call, and the workflow’s artifact upload step.
  • Found: evaluate() calls verdict_for() before it returns the result, diff, and log at products/tasks/evals/golden_prs/__main__.py:55-58. judge() makes the Anthropic request without handling request errors at products/tasks/evals/golden_prs/scoring.py:103-119. main() calls write_result() only after evaluate() returns at products/tasks/evals/golden_prs/__main__.py:143-144. The workflow upload runs even after a failure, but uploads only files already present in the results directory at .github/workflows/golden-pr-evals.yml:166-172.
  • Impact: If the judge request raises, the completed agent diff and log are not persisted. The workflow cannot preserve those artifacts, and the judge cannot be retried without running the agent again.
Suggested fix

Persist the candidate diff and agent log before calling the judge, or catch judge failures and write a result that records the judge error while retaining the deterministic scores and agent artifacts.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/__main__.py#L58

<issue_description>
If the Anthropic request in `judge()` fails after its retries, `evaluate()` raises before returning the candidate diff and agent log. The caller therefore never reaches `write_result()`, and the workflow's always-run artifact upload has no completed agent output to preserve. This discards work already spent running the agent and prevents retrying only the judge.
</issue_description>

<issue_validation>
- **Checked:** Traced `evaluate()`, `judge()`, the result-writing call, and the workflow’s artifact upload step.
- **Found:** `evaluate()` calls `verdict_for()` before it returns the result, diff, and log at `products/tasks/evals/golden_prs/__main__.py:55-58`. `judge()` makes the Anthropic request without handling request errors at `products/tasks/evals/golden_prs/scoring.py:103-119`. `main()` calls `write_result()` only after `evaluate()` returns at `products/tasks/evals/golden_prs/__main__.py:143-144`. The workflow upload runs even after a failure, but uploads only files already present in the results directory at `.github/workflows/golden-pr-evals.yml:166-172`.
- **Impact:** If the judge request raises, the completed agent diff and log are not persisted. The workflow cannot preserve those artifacts, and the judge cannot be retried without running the agent again.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Persist the candidate diff and agent log before calling the judge, or catch judge failures and write a result that records the judge error while retaining the deterministic scores and agent artifacts.
</potential_solution>

header = DIFF_HEADER.match(line)
if header:
counting = not is_artifact(header.group(2))
elif counting and line.startswith("+") and not line.startswith("+++"):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

Count added lines that begin with ++

consider bug

Issue description

This condition skips every added diff line that starts with +++, not only the +++ b/... file header. A valid source line such as ++counter is therefore omitted from the added-line score, which can distort the reported F1.

Why we think it's a valid issue
  • Checked: Traced added_lines and searched the repository for valid source lines that begin with ++ after the diff prefix.
  • Found: products/tasks/evals/golden_prs/scoring.py:60 excludes every added line whose diff text starts with +++. The repository contains valid TypeScript lines such as ++index in frontend/src/scenes/terminal/terminalHogql.ts:150. Another diff parser explicitly preserves content like ++i; in products/desktop/packages/agent/src/adapters/codex-app-server/mapping.ts:162.
  • Impact: When such a line appears in a candidate or golden diff, added_lines omits it and can distort the reported added_line_f1 score. The trigger is uncommon, so the reported consider priority is appropriate.
Suggested fix

Exclude only the actual +++ b/... file header, or track whether the parser is inside a hunk before counting added lines.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/scoring.py#L60

<issue_description>
This condition skips every added diff line that starts with `+++`, not only the `+++ b/...` file header. A valid source line such as `++counter` is therefore omitted from the added-line score, which can distort the reported F1.
</issue_description>

<issue_validation>
- **Checked:** Traced `added_lines` and searched the repository for valid source lines that begin with `++` after the diff prefix.
- **Found:** `products/tasks/evals/golden_prs/scoring.py:60` excludes every added line whose diff text starts with `+++`. The repository contains valid TypeScript lines such as `++index` in `frontend/src/scenes/terminal/terminalHogql.ts:150`. Another diff parser explicitly preserves content like `++i;` in `products/desktop/packages/agent/src/adapters/codex-app-server/mapping.ts:162`.
- **Impact:** When such a line appears in a candidate or golden diff, `added_lines` omits it and can distort the reported `added_line_f1` score. The trigger is uncommon, so the reported `consider` priority is appropriate.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Exclude only the actual `+++ b/...` file header, or track whether the parser is inside a hunk before counting added lines.
</potential_solution>

Comment thread .github/workflows/golden-pr-evals.yml
Comment thread products/tasks/evals/golden_prs/__main__.py
Comment thread .github/workflows/golden-pr-evals.yml
Comment thread .github/workflows/golden-pr-evals.yml Outdated
Comment thread products/tasks/evals/golden_prs/__main__.py

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
products/tasks/evals/golden_prs/README.md-27-28 (1)

27-28: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

State the Claude judge prerequisite.

If a devbox has codex but no claude or ANTHROPIC_API_KEY, the agent can run but the judge fails afterward. State that a Codex run also needs an API key or a signed-in claude CLI.

🧹 Nitpick comments (1)
products/tasks/evals/golden_prs/test_golden_prs.py (1)

185-185: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Scrub repository-location variables in temporary-repository tests. An inherited GIT_DIR or GIT_WORK_TREE can redirect the Git commands despite their cwd.

  • products/tasks/evals/golden_prs/test_golden_prs.py#L185-L185: use a scrubbed environment for the prefix-test Git commands; keep the GIT_CONFIG_* settings under test.
  • products/tasks/evals/golden_prs/test_golden_prs.py#L196-L196: use a scrubbed environment for the checkout-test Git commands, including commit and revision lookup.
    Based on learnings: tests that spawn Git against temporary repositories should scrub repository-location variables.

Source: Learnings


ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: 1942c479-d703-4b88-a2f4-21a2094c0660

📥 Commits

Reviewing files that changed from the base of the PR and between 0be288d and fce1687.

📒 Files selected for processing (4)
  • products/tasks/evals/golden_prs/README.md
  • products/tasks/evals/golden_prs/scoring.py
  • products/tasks/evals/golden_prs/test_golden_prs.py
  • products/tasks/evals/golden_prs/workspace.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 7 remain after this review.

Comment thread products/tasks/evals/golden_prs/scoring.py Outdated
Comment thread products/tasks/evals/golden_prs/scoring.py
Comment thread products/tasks/evals/golden_prs/workspace.py Outdated

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

PostHog Review

Found 2 should fix, 1 consider.

Comment thread products/tasks/evals/golden_prs/__main__.py Outdated

# Generated or binary files: an agent cannot regenerate them without running the test suite,
# so they must not count against it.
ARTIFACT_SUFFIXES = (".ambr", ".snap", ".png", ".jpg", ".jpeg", ".gif", ".webp", ".ico")

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

Exclude SVG images from deterministic scores

consider bug

Issue description

ARTIFACT_SUFFIXES does not include .svg, so is_artifact() counts SVG assets as changed source files and added_lines() scores their markup. When a golden PR changes an SVG, the file recall and line F1 can penalize an agent for not reproducing an image even though the evaluation says to ignore images.

Why we think it's a valid issue
  • Checked: Traced is_artifact() through changed_files() and added_lines() in products/tasks/evals/golden_prs/scoring.py:49, products/tasks/evals/golden_prs/scoring.py:53, and products/tasks/evals/golden_prs/scoring.py:57. Checked the documented image-exclusion behavior in products/tasks/evals/golden_prs/README.md:20 and the current golden PR files.
  • Found: ARTIFACT_SUFFIXES at products/tasks/evals/golden_prs/scoring.py:16 omits .svg. Both deterministic scoring paths use is_artifact(), so an SVG diff contributes to file overlap and added-line F1. The current golden PRs do not include SVG files, but the documented workflow supports adding more cases.
  • Impact: Adding a golden case that changes an SVG would make the deterministic scores count image markup, contrary to the documented image-exclusion behavior.
  • Priority: Lowered to consider because no current golden case is affected; the scoring mismatch applies when an SVG-bearing case is added.
Suggested fix

Add .svg and any other supported image formats to ARTIFACT_SUFFIXES, and cover the exclusion in the scoring tests.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/scoring.py#L16

<issue_description>
`ARTIFACT_SUFFIXES` does not include `.svg`, so `is_artifact()` counts SVG assets as changed source files and `added_lines()` scores their markup. When a golden PR changes an SVG, the file recall and line F1 can penalize an agent for not reproducing an image even though the evaluation says to ignore images.
</issue_description>

<issue_validation>
- **Checked:** Traced `is_artifact()` through `changed_files()` and `added_lines()` in `products/tasks/evals/golden_prs/scoring.py:49`, `products/tasks/evals/golden_prs/scoring.py:53`, and `products/tasks/evals/golden_prs/scoring.py:57`. Checked the documented image-exclusion behavior in `products/tasks/evals/golden_prs/README.md:20` and the current golden PR files.
- **Found:** `ARTIFACT_SUFFIXES` at `products/tasks/evals/golden_prs/scoring.py:16` omits `.svg`. Both deterministic scoring paths use `is_artifact()`, so an SVG diff contributes to file overlap and added-line F1. The current golden PRs do not include SVG files, but the documented workflow supports adding more cases.
- **Impact:** Adding a golden case that changes an SVG would make the deterministic scores count image markup, contrary to the documented image-exclusion behavior.
- **Priority:** Lowered to `consider` because no current golden case is affected; the scoring mismatch applies when an SVG-bearing case is added.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Add `.svg` and any other supported image formats to `ARTIFACT_SUFFIXES`, and cover the exclusion in the scoring tests.
</potential_solution>

Comment thread products/tasks/evals/golden_prs/scoring.py Outdated
@pauldambra

Copy link
Copy Markdown
Member Author

🤖 Robot comment: first full end-to-end run of the golden PR evals, from a Mac with the signed-in claude and codex CLIs. All 12 cases ran for both agents. The judge was claude-opus-5 through the CLI fallback in this branch.

claude claude-opus-5

PR Title Author Files hit Line F1 Judge Minutes Cost $
#18481 fix(retention): Properly apply Monday as the first day of the week Twixes 1.00 0.57 0.95 5.0 1.63
#18522 feat(hogql): better debug errors mariusandra 1.00 0.44 0.95 7.4 1.90
#18829 feat: hide seekbar preview for long recordings pauldambra 0.40 0.36 0.60 3.1 0.84
#19256 feat: Show activity panel if there are any unread notifications benjackwhite 1.00 0.77 0.90 4.1 1.72
#19390 fix: replay events table had duplicate columns pauldambra 0.50 0.00 0.60 4.0 1.55
#24988 fix(projects): Fix project creation when access_control set Twixes 0.50 0.28 0.70 3.2 1.47
#25310 feat(persons): save persons as a static cohort mariusandra 0.00 0.20 0.85 7.5 2.98
#25334 fix: replay will send nonsense strings that should be captured pauldambra 0.20 0.20 0.80 11.2 4.30
#25349 fix(apps): transpile site apps on create mariusandra 1.00 0.27 0.85 5.9 2.73
#25832 fix(events): Account for project timezone in absolute range EventsQuery Twixes 1.00 0.36 0.95 1.8 0.69
#26792 feat: Added manual remoteconfig sync command benjackwhite 0.50 0.40 0.70 2.1 0.93
#26893 fix: Email validation tokens benjackwhite 1.00 0.17 0.90 11.1 6.04
Mean 0.68 0.33 0.81

codex gpt-5.5

PR Title Author Files hit Line F1 Judge Minutes Cost $
#18481 fix(retention): Properly apply Monday as the first day of the week Twixes 1.00 0.20 0.90 12.8 0.00
#18522 feat(hogql): better debug errors mariusandra 0.17 0.19 0.30 3.7 0.00
#18829 feat: hide seekbar preview for long recordings pauldambra 0.40 0.23 0.45 3.0 0.00
#19256 feat: Show activity panel if there are any unread notifications benjackwhite 0.40 0.28 0.70 2.8 0.00
#19390 fix: replay events table had duplicate columns pauldambra 0.50 0.00 0.60 2.1 0.00
#24988 fix(projects): Fix project creation when access_control set Twixes 0.25 0.13 0.70 3.2 0.00
#25310 feat(persons): save persons as a static cohort mariusandra 0.00 0.25 0.75 3.6 0.00
#25334 fix: replay will send nonsense strings that should be captured pauldambra 0.40 0.21 0.70 3.8 0.00
#25349 fix(apps): transpile site apps on create mariusandra 0.67 0.30 0.85 4.2 0.00
#25832 fix(events): Account for project timezone in absolute range EventsQuery Twixes 1.00 0.14 0.95 1.6 0.00
#26792 feat: Added manual remoteconfig sync command benjackwhite 0.50 0.53 0.75 2.4 0.00
#26893 fix: Email validation tokens benjackwhite 1.00 0.38 0.92 4.4 0.00
Mean 0.52 0.24 0.71

Codex cost shows 0.00 because the runner does not parse usage from the codex exec --json event stream yet.

What each agent got right or wrong

Scores that look wrong for the change

Known follow-ups, not in this run: parse Codex usage and cost, verify the SDK judge path with an API key set, and stop the agents from running the eval workspace's tests with the host venv.

@pauldambra

Copy link
Copy Markdown
Member Author

🤖 Robot comment: second golden PR run, this time with the two Claude and two GPT models we want to compare: claude-opus-5-5, claude-fable-5-1, gpt-6-sol and gpt-6-astra. Same setup as the first comment: signed-in claude and codex CLIs, judge claude-opus-5 through the CLI fallback. All 12 cases ran for all four.

claude claude-opus-5-5

PR Title Author Files hit Line F1 Judge Minutes Cost $
#18481 fix(retention): Properly apply Monday as the first day of the week Twixes 1.00 0.56 0.90 1.6 0.58
#18522 feat(hogql): better debug errors mariusandra 0.83 0.33 0.55 1.6 0.58
#18829 feat: hide seekbar preview for long recordings pauldambra 0.40 0.36 0.60 1.8 0.28
#19256 feat: Show activity panel if there are any unread notifications benjackwhite 0.80 0.68 0.85 1.1 0.46
#19390 fix: replay events table had duplicate columns pauldambra 1.00 0.00 0.30 1.2 0.44
#24988 fix(projects): Fix project creation when access_control set Twixes 0.50 0.69 0.80 0.9 0.46
#25310 feat(persons): save persons as a static cohort mariusandra 0.00 0.31 0.65 0.8 0.37
#25334 fix: replay will send nonsense strings that should be captured pauldambra 0.20 0.16 0.80 4.7 0.47
#25349 fix(apps): transpile site apps on create mariusandra 1.00 0.33 0.90 1.4 0.71
#25832 fix(events): Account for project timezone in absolute range EventsQuery Twixes 1.00 0.33 0.95 0.7 0.27
#26792 feat: Added manual remoteconfig sync command benjackwhite 1.00 0.80 0.95 0.5 0.27
#26893 fix: Email validation tokens benjackwhite 1.00 0.34 0.90 1.5 0.54
Mean 0.73 0.41 0.76

claude claude-fable-5-1

PR Title Author Files hit Line F1 Judge Minutes Cost $
#18481 fix(retention): Properly apply Monday as the first day of the week Twixes 1.00 0.52 0.85 2.5 1.73
#18522 feat(hogql): better debug errors mariusandra 1.00 0.50 0.95 2.9 2.10
#18829 feat: hide seekbar preview for long recordings pauldambra 0.40 0.30 0.40 1.0 0.91
#19256 feat: Show activity panel if there are any unread notifications benjackwhite 0.60 0.31 0.30 5.4 2.76
#19390 fix: replay events table had duplicate columns pauldambra 0.50 0.00 0.70 1.3 1.07
#24988 fix(projects): Fix project creation when access_control set Twixes 0.50 0.61 0.80 3.3 2.25
#25310 feat(persons): save persons as a static cohort mariusandra 0.00 0.17 0.80 3.3 2.23
#25334 fix: replay will send nonsense strings that should be captured pauldambra 0.40 0.25 0.80 8.5 3.72
#25349 fix(apps): transpile site apps on create mariusandra 1.00 0.29 0.80 21.7 4.82
#25832 fix(events): Account for project timezone in absolute range EventsQuery Twixes 1.00 0.31 0.95 1.3 0.84
#26792 feat: Added manual remoteconfig sync command benjackwhite 1.00 0.60 0.60 1.2 0.70
#26893 fix: Email validation tokens benjackwhite 1.00 0.21 0.90 5.7 1.72
Mean 0.70 0.34 0.74

codex gpt-6-sol

PR Title Author Files hit Line F1 Judge Minutes Cost $
#18481 fix(retention): Properly apply Monday as the first day of the week Twixes 1.00 0.25 0.85 1.7 0.00
#18522 feat(hogql): better debug errors mariusandra 0.33 0.21 0.45 2.5 0.00
#18829 feat: hide seekbar preview for long recordings pauldambra 0.60 0.33 0.70 1.6 0.00
#19256 feat: Show activity panel if there are any unread notifications benjackwhite 0.60 0.27 0.80 2.5 0.00
#19390 fix: replay events table had duplicate columns pauldambra 1.00 0.83 0.40 1.7 0.00
#24988 fix(projects): Fix project creation when access_control set Twixes 0.50 0.12 0.70 2.0 0.00
#25310 feat(persons): save persons as a static cohort mariusandra 0.00 0.22 0.70 2.1 0.00
#25334 fix: replay will send nonsense strings that should be captured pauldambra 0.40 0.17 0.70 3.0 0.00
#25349 fix(apps): transpile site apps on create mariusandra 0.67 0.19 0.65 1.8 0.00
#25832 fix(events): Account for project timezone in absolute range EventsQuery Twixes 1.00 0.14 0.95 1.3 0.00
#26792 feat: Added manual remoteconfig sync command benjackwhite 1.00 0.56 0.95 2.2 0.00
#26893 fix: Email validation tokens benjackwhite 1.00 0.13 0.95 3.0 0.00
Mean 0.68 0.29 0.73

codex gpt-6-astra

PR Title Author Files hit Line F1 Judge Minutes Cost $
#18481 fix(retention): Properly apply Monday as the first day of the week Twixes 1.00 0.18 0.90 1.5 0.00
#18522 feat(hogql): better debug errors mariusandra 1.00 0.28 0.75 1.8 0.00
#18829 feat: hide seekbar preview for long recordings pauldambra 0.40 0.34 0.65 1.1 0.00
#19256 feat: Show activity panel if there are any unread notifications benjackwhite 0.60 0.39 0.70 1.8 0.00
#19390 fix: replay events table had duplicate columns pauldambra 1.00 0.83 0.85 1.7 0.00
#24988 fix(projects): Fix project creation when access_control set Twixes 0.75 0.24 0.65 2.1 0.00
#25310 feat(persons): save persons as a static cohort mariusandra 0.00 0.23 0.75 2.5 0.00
#25334 fix: replay will send nonsense strings that should be captured pauldambra 0.20 0.17 0.75 3.4 0.00
#25349 fix(apps): transpile site apps on create mariusandra 1.00 0.41 0.65 2.2 0.00
#25832 fix(events): Account for project timezone in absolute range EventsQuery Twixes 1.00 0.23 0.95 1.0 0.00
#26792 feat: Added manual remoteconfig sync command benjackwhite 0.50 0.68 0.75 1.2 0.00
#26893 fix: Email validation tokens benjackwhite 1.00 0.19 0.95 2.3 0.00
Mean 0.70 0.35 0.78

Codex cost still shows 0.00 because the runner does not parse usage from the codex exec --json event stream yet.

Mean scores across all six runs

Agent Files hit Line F1 Judge
claude-opus-5 0.68 0.33 0.81
gpt-6-astra 0.70 0.35 0.78
claude-opus-5-5 0.73 0.41 0.76
claude-fable-5-1 0.70 0.34 0.74
gpt-6-sol 0.68 0.29 0.73
gpt-5.5 0.52 0.24 0.71

The four new models sit close together. Opus 5.5 hits the right files and lines most often, and was the fastest and cheapest Claude run. Fable 5.1 costs about three times more per case than Opus 5.5 for a lower judge score.

What each agent got right or wrong

Scores that look wrong for the change

The earlier follow-ups still stand: parse Codex usage and cost, verify the SDK judge path with an API key set, and stop the agents from setting up an environment in the eval workspace. The last one cost the Claude agents most of their time on some cases.

@pauldambra
pauldambra marked this pull request as ready for review September 27, 2026 17:38
@chatgpt-codex-connector

chatgpt-codex-connector Bot commented Sep 27, 2026 •

Copy link
Copy Markdown

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

Review Status Commit Review trigger
📝 Code Review ✅ Completed 2026-09-27T17:46:29.248303Z fce1687 Draft marked ready
ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review" or "@codex security review".

Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings.

@pr-assigner-resolver-posthog
pr-assigner-resolver-posthog Bot requested review from a team September 27, 2026 17:38

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: fce1687376

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread products/tasks/evals/golden_prs/agents.py Outdated
Comment thread .github/workflows/golden-pr-evals.yml
Comment thread products/tasks/evals/golden_prs/workspace.py Outdated
with checkout_parent(repo, pr) as workdir:
run = run_agent(runtime, model, prompt, workdir, timeout_seconds)
candidate = candidate_diff(workdir)
verdict = verdict_for(run, prompt, candidate, golden, judge_model)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Persist artifacts before invoking the judge

If the judge raises—for example because the dispatch supplied an invalid judge model or the Anthropic request fails—the already-collected candidate diff and agent log exist only in memory here, and write_result() is never reached. The workspace has also been deleted by this point, so the artifact upload has no diagnostic files and the report simply omits this case. Persist the raw outputs before judging or convert judge exceptions into an explicit result.

Useful? React with 👍 / 👎.

@pauldambra pauldambra left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Robot comment: QA Swarm review complete. See inline comments and the summary comment.

Comment thread products/tasks/evals/golden_prs/workspace.py Outdated
Comment thread products/tasks/evals/golden_prs/agents.py
Comment thread products/tasks/evals/golden_prs/workspace.py Outdated
Comment thread products/tasks/evals/golden_prs/agents.py Outdated
@pauldambra

pauldambra commented Sep 27, 2026 •

Copy link
Copy Markdown
Member Author

🤖 Robot comment:

Note

🤖 Automated comment by QA Swarm — not written by a human

Multi-perspective review: router (cheap-first pass) + delegated reviewers (qa-team, paul-reviewer, xp-reviewer, security-audit as warranted)

Verdict: ✅ APPROVE (round 3 @ 678729f)

Round 3 reviewed only the delta since round 2 (0a33f1e..678729f: version probe before the run, fixed baseline ref, secrets withheld from the staging git call, bounded archive extraction, symlink and mode restore, base-10 timeout parsing). Nothing actionable.

Key findings

  • ⚪ NIT workspace.py:107 - content.decode() on a symlink target assumes UTF-8, which is the same assumption git makes for paths. Acceptable for eval tooling.

Convergence

None.

Reviewer summaries

Reviewer Assessment
🧭 router (haiku) Verified git diff --cached <ref> semantics, the namespaced baseline ref, the tar and archive timeouts, the force-add rationale, and that chmod only touches restored files. Danger LOW, confidence HIGH, no delegation.
Previous rounds (2)

round 1 @ fce1687 — ⚠️ REQUEST CHANGES: one HIGH (PR number in workspace commit message and tmpdir), two MEDIUM (unsandboxed agent can reach the adjacent checkout; unchecked git show), one LOW, one NIT.
round 2 @ 0a33f1e — ✅ APPROVE: delta reviewed, no new findings; round-1 HIGH and git show MEDIUM fixed, sandbox limit documented in the README.


Automated by QA Swarm — not a human review

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

PostHog Review

Found 5 should fix, 1 consider.

Comment thread products/tasks/evals/golden_prs/workspace.py Outdated
Comment thread products/tasks/evals/golden_prs/__main__.py
Comment thread products/tasks/evals/golden_prs/workspace.py Outdated
Comment thread products/tasks/evals/golden_prs/workspace.py Outdated
Comment thread .github/workflows/golden-pr-evals.yml Outdated
Comment on lines +43 to +45
if failure and not candidate.strip():
return Verdict(score=0.0, reasoning=f"The agent failed before changing any file: {failure}")
return judge(prompt, candidate, golden, model=judge_model)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

Report failed agent runs as failures, not normal scores

should_fix bug

Issue description

When an agent exits unsuccessfully after changing files, this code still asks the judge to score its diff. The report does not show the non-zero exit, so it presents that score as a normal completed run. When the failed agent changed nothing, the verdict is zero and the report includes it in the mean. Both outcomes can misrepresent an agent failure as its evaluation score.

Why we think it's a valid issue
  • Checked: Traced failure handling in products/tasks/evals/golden_prs/__main__.py and products/tasks/evals/golden_prs/agents.py, including how report() formats rows and calculates means.
  • Found: verdict_for() calls the judge for any non-empty candidate, even when agent_failure(run) returns a failure at products/tasks/evals/golden_prs/__main__.py:41-45. CaseResult records exit_code at products/tasks/evals/golden_prs/__main__.py:67-72, but report() does not display it; it marks timeouts only at products/tasks/evals/golden_prs/__main__.py:95-100.
  • Found: The report calculates its file, line, and judge means across every result at products/tasks/evals/golden_prs/__main__.py:103-105. A failed run with no candidate receives a zero verdict at products/tasks/evals/golden_prs/__main__.py:42-44 and remains in those means.
  • Impact: The summary does not distinguish failed runs from completed evaluations, so readers can mistake partial or failed attempts for comparable scores. Including failed runs in the means can distort comparisons between agents.
Suggested fix

Record and display the agent failure as a separate run status. Do not include failed runs as ordinary judge scores in the report mean; if you keep a score for a partial diff, label it clearly as a failed run.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/__main__.py#L43-45

<issue_description>
When an agent exits unsuccessfully after changing files, this code still asks the judge to score its diff. The report does not show the non-zero exit, so it presents that score as a normal completed run. When the failed agent changed nothing, the verdict is zero and the report includes it in the mean. Both outcomes can misrepresent an agent failure as its evaluation score.
</issue_description>

<issue_validation>
- **Checked:** Traced failure handling in `products/tasks/evals/golden_prs/__main__.py` and `products/tasks/evals/golden_prs/agents.py`, including how `report()` formats rows and calculates means.
- **Found:** `verdict_for()` calls the judge for any non-empty candidate, even when `agent_failure(run)` returns a failure at `products/tasks/evals/golden_prs/__main__.py:41-45`. `CaseResult` records `exit_code` at `products/tasks/evals/golden_prs/__main__.py:67-72`, but `report()` does not display it; it marks timeouts only at `products/tasks/evals/golden_prs/__main__.py:95-100`.
- **Found:** The report calculates its file, line, and judge means across every result at `products/tasks/evals/golden_prs/__main__.py:103-105`. A failed run with no candidate receives a zero verdict at `products/tasks/evals/golden_prs/__main__.py:42-44` and remains in those means.
- **Impact:** The summary does not distinguish failed runs from completed evaluations, so readers can mistake partial or failed attempts for comparable scores. Including failed runs in the means can distort comparisons between agents.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Record and display the agent failure as a separate run status. Do not include failed runs as ordinary judge scores in the report mean; if you keep a score for a partial diff, label it clearly as a failed run.
</potential_solution>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
.github/workflows/golden-pr-evals.yml-165-170 (1)

165-170: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Normalize CASE_TIMEOUT_MINUTES to base 10 before arithmetic expansion.

The validation accepts 08 and 010, but Bash arithmetic treats leading-zero values as octal. 08 can fail, and 010 becomes 8 minutes instead of 10.

Suggested fix
                   esac
+                  CASE_TIMEOUT_MINUTES=$((10#$CASE_TIMEOUT_MINUTES))
                   if [ "$CASE_TIMEOUT_MINUTES" -lt 1 ] || [ "$CASE_TIMEOUT_MINUTES" -gt 45 ]; then

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: 1b8629b9-6fe6-4253-bc89-47fdb00f0fd5

📥 Commits

Reviewing files that changed from the base of the PR and between fce1687 and 0a33f1e.

📒 Files selected for processing (9)
  • .github/workflows/golden-pr-evals.yml
  • products/tasks/evals/golden_prs/README.md
  • products/tasks/evals/golden_prs/__main__.py
  • products/tasks/evals/golden_prs/agents.py
  • products/tasks/evals/golden_prs/cases.py
  • products/tasks/evals/golden_prs/golden_prs.json
  • products/tasks/evals/golden_prs/scoring.py
  • products/tasks/evals/golden_prs/test_golden_prs.py
  • products/tasks/evals/golden_prs/workspace.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 8 remain after this review.

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

PostHog Review

Found 2 must fix, 2 should fix.

Comment on lines +11 to +12
workflow_dispatch:
inputs:

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

[must_fix] Restrict provider secrets to trusted workflow refs

must_fix security

Issue description

This workflow can be dispatched against a non-default branch, and GitHub runs the workflow version from that ref. A modified workflow on that branch could read and exfiltrate the repository secrets used by the evaluation, even before it starts the agent. The workflow_dispatch branch selector and ref behavior make this a separate exposure path from the agent prompt. (docs.github.com)

Why we think it's a valid issue
  • Checked: Reviewed .github/workflows/golden-pr-evals.yml from dispatch through the evaluation job, including its secret checks and provider-key use.
  • Found: The workflow allows workflow_dispatch at .github/workflows/golden-pr-evals.yml:11. The evaluate job reads repository secrets at .github/workflows/golden-pr-evals.yml:92-93 and passes both provider keys to the eval step at .github/workflows/golden-pr-evals.yml:153-154. The job does not declare a protected environment.
  • Impact: A modified workflow on a dispatched ref can change its steps and access the repository secrets. persist-credentials: false does not restrict access to these explicitly injected provider keys. Keeping the keys only as environment secrets, with deployment limited to trusted refs, prevents a branch from accessing them without the required environment binding and approval.
  • Priority: must_fix is appropriate because this exposes paid provider credentials to workflow code on an untrusted ref.
Suggested fix

Store the provider keys in a protected GitHub environment that only allows the trusted default branch, and bind the evaluate job to that environment. Add required approval if appropriate. A guard in this workflow alone is not sufficient because a modified branch can remove it.

Prompt to fix with AI (copy-paste)
## Context
@.github/workflows/golden-pr-evals.yml#L11-12

<issue_description>
This workflow can be dispatched against a non-default branch, and GitHub runs the workflow version from that ref. A modified workflow on that branch could read and exfiltrate the repository secrets used by the evaluation, even before it starts the agent. The `workflow_dispatch` branch selector and ref behavior make this a separate exposure path from the agent prompt. ([docs.github.com](https://docs.github.com/en/actions/reference/workflows-and-actions/events-that-trigger-workflows?utm_source=openai))
</issue_description>

<issue_validation>
- **Checked:** Reviewed `.github/workflows/golden-pr-evals.yml` from dispatch through the evaluation job, including its secret checks and provider-key use.
- **Found:** The workflow allows `workflow_dispatch` at `.github/workflows/golden-pr-evals.yml:11`. The `evaluate` job reads repository secrets at `.github/workflows/golden-pr-evals.yml:92-93` and passes both provider keys to the eval step at `.github/workflows/golden-pr-evals.yml:153-154`. The job does not declare a protected environment.
- **Impact:** A modified workflow on a dispatched ref can change its steps and access the repository secrets. `persist-credentials: false` does not restrict access to these explicitly injected provider keys. Keeping the keys only as environment secrets, with deployment limited to trusted refs, prevents a branch from accessing them without the required environment binding and approval.
- **Priority:** `must_fix` is appropriate because this exposes paid provider credentials to workflow code on an untrusted ref.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Store the provider keys in a protected GitHub environment that only allows the trusted default branch, and bind the `evaluate` job to that environment. Add required approval if appropriate. A guard in this workflow alone is not sufficient because a modified branch can remove it.
</potential_solution>

Comment on lines +134 to +136
npm install -g @openai/codex
else
npm install -g @anthropic-ai/claude-code

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

[should_fix] Pin the agent CLI before exposing credentials

should_fix security

Issue description

These commands install the latest CLI version on every run. The eval later gives the selected provider key to the agent process. If a registry release is compromised or changes unexpectedly, the installed CLI can read or send that key. This also makes runs less reproducible.

Why we think it's a valid issue
  • Checked: Traced the CLI installation at .github/workflows/golden-pr-evals.yml:128-138 to the later agent launch and environment filtering in products/tasks/evals/golden_prs/agents.py:39-45.
  • Found: Both npm install -g commands omit a version, so each run installs the package version currently selected by the registry. The launched CLI receives the selected provider key through agent_environment; only the other provider key is removed.
  • Impact: A compromised or malicious newly published CLI version can access and send the provider key when the agent starts. Pinning the CLI version makes upgrades intentional and reduces this exposure. The issue also affects reproducibility because runs can use different CLI versions.
Suggested fix

Install an explicitly reviewed CLI version instead of the unqualified latest version. Keep version updates intentional and controlled.

Prompt to fix with AI (copy-paste)
## Context
@.github/workflows/golden-pr-evals.yml#L134-136

<issue_description>
These commands install the latest CLI version on every run. The eval later gives the selected provider key to the agent process. If a registry release is compromised or changes unexpectedly, the installed CLI can read or send that key. This also makes runs less reproducible.
</issue_description>

<issue_validation>
- **Checked:** Traced the CLI installation at `.github/workflows/golden-pr-evals.yml:128-138` to the later agent launch and environment filtering in `products/tasks/evals/golden_prs/agents.py:39-45`.
- **Found:** Both `npm install -g` commands omit a version, so each run installs the package version currently selected by the registry. The launched CLI receives the selected provider key through `agent_environment`; only the other provider key is removed.
- **Impact:** A compromised or malicious newly published CLI version can access and send the provider key when the agent starts. Pinning the CLI version makes upgrades intentional and reduces this exposure. The issue also affects reproducibility because runs can use different CLI versions.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Install an explicitly reviewed CLI version instead of the unqualified latest version. Keep version updates intentional and controlled.
</potential_solution>

Comment thread products/tasks/evals/golden_prs/workspace.py
Comment thread products/tasks/evals/golden_prs/workspace.py

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

PostHog Review

Found 3 should fix.

Comment on lines +68 to +78
def agent_failure(run: AgentRun) -> str | None:
"""Why the agent exited non-zero, so a login or network failure never reads as a bad attempt."""
if run.exit_code == 0:
return None
if run.timed_out:
return "The agent hit the case timeout."
report = _claude_report(run)
if report.get("is_error") and report.get("result"):
return str(report["result"])
stderr_lines = [line for line in run.stderr.splitlines() if line.strip()]
return stderr_lines[-1] if stderr_lines else f"The agent exited with code {run.exit_code}."

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

Report failures when the agent leaves a partial diff

should_fix bug

Issue description

When the agent exits with an error after changing files, agent_failure() returns a failure reason, but the runner uses it only when the candidate diff is empty. For a non-empty diff, the run is judged and the failure reason is discarded. The report can then show a normal judge score without explaining that the agent exited unsuccessfully; it only marks timeouts explicitly.

Why we think it's a valid issue
  • Checked: Traced agent_failure() through verdict_for(), CaseResult, and report() in products/tasks/evals/golden_prs/agents.py and products/tasks/evals/golden_prs/__main__.py.
  • Found: verdict_for() uses the failure reason only when the candidate diff is empty; otherwise it returns the judge verdict (products/tasks/evals/golden_prs/__main__.py:41-45). CaseResult records exit_code and timed_out, but not the failure reason (products/tasks/evals/golden_prs/__main__.py:20-38). The report marks timeouts, but does not show other non-zero exits or their reasons (products/tasks/evals/golden_prs/__main__.py:96-100).
  • Found: The existing failure test covers a non-zero exit only with an empty diff, so it does not catch this reporting gap (products/tasks/evals/golden_prs/test_golden_prs.py:175-179).
  • Impact: A non-zero exit after producing a partial diff receives a judge score, while the report gives readers no indication that the agent run failed. This can make an incomplete attempt look like a completed one in the evaluation summary.
Suggested fix

Preserve the failure reason in the case result and show it in the report for every non-zero exit. The judge can still score a partial diff, but mark that result as a failed run so readers can distinguish it from a completed attempt.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/agents.py#L68-78

<issue_description>
When the agent exits with an error after changing files, `agent_failure()` returns a failure reason, but the runner uses it only when the candidate diff is empty. For a non-empty diff, the run is judged and the failure reason is discarded. The report can then show a normal judge score without explaining that the agent exited unsuccessfully; it only marks timeouts explicitly.
</issue_description>

<issue_validation>
- **Checked:** Traced `agent_failure()` through `verdict_for()`, `CaseResult`, and `report()` in `products/tasks/evals/golden_prs/agents.py` and `products/tasks/evals/golden_prs/__main__.py`.
- **Found:** `verdict_for()` uses the failure reason only when the candidate diff is empty; otherwise it returns the judge verdict (`products/tasks/evals/golden_prs/__main__.py:41-45`). `CaseResult` records `exit_code` and `timed_out`, but not the failure reason (`products/tasks/evals/golden_prs/__main__.py:20-38`). The report marks timeouts, but does not show other non-zero exits or their reasons (`products/tasks/evals/golden_prs/__main__.py:96-100`).
- **Found:** The existing failure test covers a non-zero exit only with an empty diff, so it does not catch this reporting gap (`products/tasks/evals/golden_prs/test_golden_prs.py:175-179`).
- **Impact:** A non-zero exit after producing a partial diff receives a judge score, while the report gives readers no indication that the agent run failed. This can make an incomplete attempt look like a completed one in the evaluation summary.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Preserve the failure reason in the case result and show it in the report for every non-zero exit. The judge can still score a partial diff, but mark that result as a failed run so readers can distinguish it from a completed attempt.
</potential_solution>

Comment on lines +212 to +216
- uses: actions/download-artifact@37930b1c2abaa49bbe596cd826c3c89aef350131 # v7.0.0
with:
pattern: golden-pr-*
path: ${{ env.RESULTS_DIR }}
merge-multiple: true

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

[should_fix] Mark incomplete score summaries

should_fix bug

Issue description

If an eval fails before it writes a result file, its artifact can be empty while other PR artifacts still download. The report then calculates scores and the mean from only the PRs that produced results, without marking the table as incomplete. This can make model comparisons misleading.

Why we think it's a valid issue
  • Checked: Traced PR selection, matrix execution, artifact upload and report generation in .github/workflows/golden-pr-evals.yml, plus result loading and mean calculation in products/tasks/evals/golden_prs/__main__.py.
  • Found: .github/workflows/golden-pr-evals.yml:48-49 exposes the selected PR list from plan, but the report job at .github/workflows/golden-pr-evals.yml:191-195 depends only on evaluate. The upload step at .github/workflows/golden-pr-evals.yml:183-189 runs after failure and warns when no result files exist.
  • Found: products/tasks/evals/golden_prs/__main__.py:88-89 loads only JSON files that exist. At products/tasks/evals/golden_prs/__main__.py:103-105, report calculates means across those loaded results without checking them against the selected PRs.
  • Impact: When at least one PR produces a result and another does not, the run summary presents a mean over only the available results without indicating that the selected set is incomplete. This can mislead model comparisons, so the finding meets the bar for a real reporting correctness issue.
Suggested fix

Pass the selected PR list to the report job and compare it with the downloaded results. Mark missing PRs as failed or label the summary as incomplete so readers do not treat a partial mean as a complete run.

Prompt to fix with AI (copy-paste)
## Context
@.github/workflows/golden-pr-evals.yml#L212-216

<issue_description>
If an eval fails before it writes a result file, its artifact can be empty while other PR artifacts still download. The report then calculates scores and the mean from only the PRs that produced results, without marking the table as incomplete. This can make model comparisons misleading.
</issue_description>

<issue_validation>
- **Checked:** Traced PR selection, matrix execution, artifact upload and report generation in `.github/workflows/golden-pr-evals.yml`, plus result loading and mean calculation in `products/tasks/evals/golden_prs/__main__.py`.
- **Found:** `.github/workflows/golden-pr-evals.yml:48-49` exposes the selected PR list from `plan`, but the report job at `.github/workflows/golden-pr-evals.yml:191-195` depends only on `evaluate`. The upload step at `.github/workflows/golden-pr-evals.yml:183-189` runs after failure and warns when no result files exist.
- **Found:** `products/tasks/evals/golden_prs/__main__.py:88-89` loads only JSON files that exist. At `products/tasks/evals/golden_prs/__main__.py:103-105`, `report` calculates means across those loaded results without checking them against the selected PRs.
- **Impact:** When at least one PR produces a result and another does not, the run summary presents a mean over only the available results without indicating that the selected set is incomplete. This can mislead model comparisons, so the finding meets the bar for a real reporting correctness issue.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Pass the selected PR list to the report job and compare it with the downloaded results. Mark missing PRs as failed or label the summary as incomplete so readers do not treat a partial mean as a complete run.
</potential_solution>

return 0
selected = select_golden_prs(golden_prs, args.pr) if args.pr else golden_prs
model = args.model or DEFAULT_MODELS[args.runtime]
results_dir = args.results_dir / f"{datetime.now(UTC):%Y%m%dT%H%M%S}-{args.runtime}-{model}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

[should_fix] Prevent model input from escaping the results directory

should_fix

Issue description

The workflow accepts inputs.model as an unrestricted string and passes it here. A model value containing path separators and .. can make results_dir resolve outside args.results_dir. write_result() then writes the PR result files at that location, so an input can overwrite files on the runner instead of only creating evaluation artifacts.

Why we think it's a valid issue
  • Checked: Traced --model through parse_args() and main(), and checked the workflow input. The CLI accepts any model string at products/tasks/evals/golden_prs/__main__.py:125, and the workflow declares model as an unrestricted string at .github/workflows/golden-pr-evals.yml:20-23.
  • Found: products/tasks/evals/golden_prs/__main__.py:147 interpolates the model into results_dir without validation. A model containing enough /.. components makes the resulting path resolve outside args.results_dir. write_result() creates that directory and writes three files there at products/tasks/evals/golden_prs/__main__.py:81-85.
  • Impact: A reachable workflow dispatch or local CLI run can write evaluation result files outside the configured results directory. This is a path traversal security issue, so the suggested change is worth addressing.
Suggested fix

Keep the supplied model value for the agent and result metadata, but use a sanitized slug or fixed run ID for the directory name. Also verify the resolved path stays inside the configured results directory.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/__main__.py#L147

<issue_description>
The workflow accepts `inputs.model` as an unrestricted string and passes it here. A model value containing path separators and `..` can make `results_dir` resolve outside `args.results_dir`. `write_result()` then writes the PR result files at that location, so an input can overwrite files on the runner instead of only creating evaluation artifacts.
</issue_description>

<issue_validation>
- **Checked:** Traced `--model` through `parse_args()` and `main()`, and checked the workflow input. The CLI accepts any model string at `products/tasks/evals/golden_prs/__main__.py:125`, and the workflow declares `model` as an unrestricted string at `.github/workflows/golden-pr-evals.yml:20-23`.
- **Found:** `products/tasks/evals/golden_prs/__main__.py:147` interpolates the model into `results_dir` without validation. A model containing enough `/..` components makes the resulting path resolve outside `args.results_dir`. `write_result()` creates that directory and writes three files there at `products/tasks/evals/golden_prs/__main__.py:81-85`.
- **Impact:** A reachable workflow dispatch or local CLI run can write evaluation result files outside the configured results directory. This is a path traversal security issue, so the suggested change is worth addressing.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Keep the supplied model value for the agent and result metadata, but use a sanitized slug or fixed run ID for the directory name. Also verify the resolved path stays inside the configured results directory.
</potential_solution>

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Note

Quiet mode is enabled, so only the most important comments were posted inline. Other review comments are grouped below.

🟡 Other comments (1)
products/tasks/evals/golden_prs/workspace.py-89-92 (1)

89-92: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

Parse Git tree paths with NUL delimiters.

If an export-ignored parent path contains characters that Git quotes, ls-tree returns the escaped path. The restore loop then passes that escaped path to git show, which can raise CalledProcessError and abort checkout_parent.

Suggested fix
-    entries = _git(repo, "ls-tree", "-r", ref).stdout.splitlines()
+    entries = _git(repo, "ls-tree", "-r", "-z", ref).stdout.rstrip("\0").split("\0")

ℹ️ Review info
⚙️ Run configuration

Configuration used: Repository: PostHog/posthog/.coderabbit.yaml

Review profile: QUIET

Plan: Enterprise

Run ID: ac84d0c0-2a05-47cb-8ffa-5ce4f5d673fb

📥 Commits

Reviewing files that changed from the base of the PR and between 0a33f1e and 678729f.

📒 Files selected for processing (3)
  • .github/workflows/golden-pr-evals.yml
  • products/tasks/evals/golden_prs/agents.py
  • products/tasks/evals/golden_prs/workspace.py

Included review availability: This review used your included allowance. Your plan provides up to 12 included reviews per hour; 6 remain after this review.

_git(workdir, "add", "-A", deny_secrets=True)
baseline = _git(workdir, "rev-parse", "--verify", "--quiet", BASELINE_REF, check=False)
if baseline.returncode == 0:
return _diff(workdir, "--cached", BASELINE_REF)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🛡️ Detected with Advanced Tier | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

sed -n '1,170p' products/tasks/evals/golden_prs/workspace.py
sed -n '1,120p' products/tasks/evals/golden_prs/agents.py
sed -n '40,80p' products/tasks/evals/golden_prs/__main__.py
sed -n '20,30p' products/tasks/evals/golden_prs/README.md

Repository: PostHog/posthog

Length of output: 13954


Sensitive Data Exposure

Reachability: External
Exploitability: Difficult
CWE: CWE-522 — Insufficiently Protected Credentials

Sanitize the candidate diff subprocess.

candidate_diff removes secrets only from git add; _diff uses the default environment and permits diff.external and textconv. An agent can write .git/config, causing Git to execute an agent-selected command with evaluator-held keys, such as ANTHROPIC_API_KEY during Codex runs. The command can also emit fabricated diff output and corrupt scoring. The runner is already unsandboxed with a documented soft boundary, so critical severity is overstated; this remains a major security issue.

Suggested fix
-    return _git(cwd, "diff", "--src-prefix=a/", "--dst-prefix=b/", *args).stdout
+    return _git(
+        cwd,
+        "diff",
+        "--no-ext-diff",
+        "--no-textconv",
+        "--src-prefix=a/",
+        "--dst-prefix=b/",
+        *args,
+        deny_secrets=True,
+    ).stdout

View in Security blast radius

@pauldambra pauldambra added the stamphog Request AI approval (no full review) label Sep 27, 2026

@stamphog stamphog Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not approved yet — waiting on the conditions below.

Re-add the stamphog label to request another review once you have addressed this.

This pull request was refused automatically before any code review took place.

Deny-list gate — FAILED (infra_cicd): the new workflow file .github/workflows/golden-pr-evals.yml matches a path pattern that stamphog never auto-clears, regardless of content.

Size gate — FAILED: 997 substantive lines across 11 files (ceiling is 800), rising to 1300 lines across 13 files including docs and generated snapshots.

Tier gate — FAILED: classified as T2-never, since it spans two distinct areas (CI infra and product code under products/tasks/evals) as a feature-sized change.

Because CI/infra changes are excluded from auto-review, this pull request needs a human reviewer regardless of size; splitting the CI workflow from the products/tasks/evals code would also help future auto-review, but a human must look at the workflow file either way.

Gate mechanics and policy version
Gate Result
prerequisites ✓ all clear
deny-list ✗ matches: infra_cicd
size ✗ too large for auto-review (997L substantive in global — ceiling is 800L; 997L, 11F total, 2 binary; 1300L/13F incl. docs/generated/snapshots)
tier ✗ classified as T2-never: T2-never (1300L, 13F, two-areas, feat)
stamphog 2.2.0 .stamphog/policy.yml @ unknown · reviewed head b915476

@stamphog stamphog Bot removed the stamphog Request AI approval (no full review) label Sep 27, 2026

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

PostHog Review

Found 3 should fix.

return 0
selected = select_golden_prs(golden_prs, args.pr) if args.pr else golden_prs
model = args.model or DEFAULT_MODELS[args.runtime]
results_dir = args.results_dir / f"{datetime.now(UTC):%Y%m%dT%H%M%S}-{args.runtime}-{model}"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

Use a unique directory for each run

should_fix bug

Issue description

The run directory uses a timestamp with only second-level precision. Two runs started in the same second with the same runtime and model use the same directory. Their files for matching PR numbers then overwrite each other, which can lose results or mix artifacts from different runs.

Why we think it's a valid issue
  • Checked: Read the full run flow and the workflow’s matrix-job setup. main() creates the run directory before evaluating cases, and write_result() writes files named by PR number.
  • Found: products/tasks/evals/golden_prs/__main__.py:147 builds the directory from a second-resolution timestamp, runtime, and model. products/tasks/evals/golden_prs/__main__.py:82 allows an existing directory, and lines 83–85 overwrite the same PR’s result, diff, and log files. The workflow uses separate matrix jobs, but concurrent local runs can share the default results directory.
  • Impact: Concurrent local runs with matching directory components and an overlapping PR can replace each other’s saved results. This is a concrete data-loss risk, so the finding meets the bar.
Suggested fix

Add a unique run identifier, such as a UUID, to the directory name and create the directory exclusively so each run keeps its own results.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/__main__.py#L147

<issue_description>
The run directory uses a timestamp with only second-level precision. Two runs started in the same second with the same runtime and model use the same directory. Their files for matching PR numbers then overwrite each other, which can lose results or mix artifacts from different runs.
</issue_description>

<issue_validation>
- **Checked:** Read the full run flow and the workflow’s matrix-job setup. `main()` creates the run directory before evaluating cases, and `write_result()` writes files named by PR number.
- **Found:** `products/tasks/evals/golden_prs/__main__.py:147` builds the directory from a second-resolution timestamp, runtime, and model. `products/tasks/evals/golden_prs/__main__.py:82` allows an existing directory, and lines 83–85 overwrite the same PR’s result, diff, and log files. The workflow uses separate matrix jobs, but concurrent local runs can share the default results directory.
- **Impact:** Concurrent local runs with matching directory components and an overlapping PR can replace each other’s saved results. This is a concrete data-loss risk, so the finding meets the bar.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Add a unique run identifier, such as a UUID, to the directory name and create the directory exclusively so each run keeps its own results.
</potential_solution>

import json, os
golden = [entry["number"] for entry in json.load(open("products/tasks/evals/golden_prs/golden_prs.json"))]
wanted = os.environ["PRS"].strip()
selected = golden if wanted == "all" else [int(n) for n in wanted.split(",") if n.strip()]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

[should_fix] Reject an empty PR selection

should_fix bug

Issue description

A blank or comma-only prs input produces an empty list. The evaluate job then skips, but the report job still runs and has no results to summarize. This can make an invalid dispatch look like a completed run with no scores.

Why we think it's a valid issue
  • Checked: .github/workflows/golden-pr-evals.yml:63-74 filters blank PR tokens but does not reject an empty selection. The plan step can therefore output [].
  • Found: .github/workflows/golden-pr-evals.yml:79-80 skips evaluate for [], but report at .github/workflows/golden-pr-evals.yml:191-195 has no corresponding selection guard. products/tasks/evals/golden_prs/__main__.py:92-94 returns No results found.\n for empty results, and products/tasks/evals/golden_prs/__main__.py:142-144 exits successfully after printing it.
  • Impact: A blank or comma-only dispatch can finish successfully without scores, so the run does not clearly signal that no PRs were evaluated. Rejecting the empty selection or skipping the report addresses this reachable input case.
Suggested fix

Fail the plan step with a clear error when selected is empty, or explicitly skip the report job when no PRs were selected.

Prompt to fix with AI (copy-paste)
## Context
@.github/workflows/golden-pr-evals.yml#L67

<issue_description>
A blank or comma-only `prs` input produces an empty list. The evaluate job then skips, but the report job still runs and has no results to summarize. This can make an invalid dispatch look like a completed run with no scores.
</issue_description>

<issue_validation>
- **Checked:** `.github/workflows/golden-pr-evals.yml:63-74` filters blank PR tokens but does not reject an empty selection. The plan step can therefore output `[]`.
- **Found:** `.github/workflows/golden-pr-evals.yml:79-80` skips `evaluate` for `[]`, but `report` at `.github/workflows/golden-pr-evals.yml:191-195` has no corresponding selection guard. `products/tasks/evals/golden_prs/__main__.py:92-94` returns `No results found.\n` for empty results, and `products/tasks/evals/golden_prs/__main__.py:142-144` exits successfully after printing it.
- **Impact:** A blank or comma-only dispatch can finish successfully without scores, so the run does not clearly signal that no PRs were evaluated. Rejecting the empty selection or skipping the report addresses this reachable input case.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Fail the plan step with a clear error when `selected` is empty, or explicitly skip the report job when no PRs were selected.
</potential_solution>

Comment on lines +162 to +164
baseline = _git(workdir, "rev-parse", "--verify", "--quiet", BASELINE_REF, check=False)
if baseline.returncode == 0:
return _diff(workdir, "--cached", BASELINE_REF)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

Keep the baseline identity outside agent-writable refs

should_fix bug

Issue description

The agent can run git update-ref refs/golden-eval/baseline <candidate-commit> inside its writable checkout. Since candidate_diff() resolves this ref after the agent exits, it can compare against the agent’s commit instead of the original baseline and produce misleading scores.

Why we think it's a valid issue
  • Checked: Traced evaluate in products/tasks/evals/golden_prs/__main__.py:55-57 and the agent command in products/tasks/evals/golden_prs/agents.py:27-34; the agent runs in the writable checkout with permission and sandbox bypasses before candidate_diff runs.
  • Found: products/tasks/evals/golden_prs/workspace.py:154 stores the baseline in a normal writable Git ref. products/tasks/evals/golden_prs/workspace.py:162-164 resolves and uses that ref after the agent exits. The agent can move it with git update-ref.
  • Impact: If the agent moves the ref to its candidate commit, the diff can omit its changes and produce incorrect evaluation scores. Passing the baseline SHA captured before the agent starts avoids trusting an agent-writable ref.
Suggested fix

Capture the baseline commit SHA before starting the agent and pass that SHA to candidate_diff() instead of resolving a ref the agent can move.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/workspace.py#L162-164

<issue_description>
The agent can run `git update-ref refs/golden-eval/baseline <candidate-commit>` inside its writable checkout. Since `candidate_diff()` resolves this ref after the agent exits, it can compare against the agent’s commit instead of the original baseline and produce misleading scores.
</issue_description>

<issue_validation>
- **Checked:** Traced `evaluate` in `products/tasks/evals/golden_prs/__main__.py:55-57` and the agent command in `products/tasks/evals/golden_prs/agents.py:27-34`; the agent runs in the writable checkout with permission and sandbox bypasses before `candidate_diff` runs.
- **Found:** `products/tasks/evals/golden_prs/workspace.py:154` stores the baseline in a normal writable Git ref. `products/tasks/evals/golden_prs/workspace.py:162-164` resolves and uses that ref after the agent exits. The agent can move it with `git update-ref`.
- **Impact:** If the agent moves the ref to its candidate commit, the diff can omit its changes and produce incorrect evaluation scores. Passing the baseline SHA captured before the agent starts avoids trusting an agent-writable ref.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Capture the baseline commit SHA before starting the agent and pass that SHA to `candidate_diff()` instead of resolving a ref the agent can move.
</potential_solution>

@pauldambra
pauldambra added this pull request to stack #107468 September 27, 2026 21:10
Score how well a coding agent one-shots a human-written PostHog PR from 2023 or 2024. The runner checks out the commit before the PR, prompts the agent with the PR description, and compares the diff with the merged PR. A workflow_dispatch workflow runs it on demand, one job per PR.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 5eb47e0a-d342-4b0d-9c45-839c78360e9a
…tempt

A dry run with a logged-out CLI scored 0 with "the agent changed no files", which reads like the model failed the task. The result now carries the CLI's own error, and the judge is not called for a run that failed before changing anything.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Generated-By: PostHog Desktop
Task-Id: 5eb47e0a-d342-4b0d-9c45-839c78360e9a
The judge falls back to the signed-in claude CLI when ANTHROPIC_API_KEY
is not set, so a devbox does not need an API key. The candidate and
golden diffs force a/ and b/ prefixes, because a host diff.mnemonicPrefix
or diff.noprefix setting made the scorer miss every file. The agent
workspace gets back the files that git archive drops as export-ignore,
because without .gitignore the agent's build output was staged into the
candidate diff.

Generated-By: PostHog Desktop
Task-Id: 334f773a-ab58-4546-98cf-39d3f79448cc
Keep the golden PR number out of the agent workspace, drop the other
provider key and GitHub tokens from the agent environment, bound every
subprocess with a timeout, restore directory-level export-ignored files,
validate the case timeout input, and strip tracker references from the
fixture PR bodies.

Generated-By: PostHog Desktop
Task-Id: 334f773a-ab58-4546-98cf-39d3f79448cc
Generated-By: PostHog Desktop
Task-Id: 334f773a-ab58-4546-98cf-39d3f79448cc
Probe the agent CLI version before the run, bound the archive extraction,
keep ignored tracked files and symlinks in the baseline, score the
candidate diff against a fixed baseline ref, withhold secrets from the
staging git call, and parse the case timeout as base 10.

Generated-By: PostHog Desktop
Task-Id: 334f773a-ab58-4546-98cf-39d3f79448cc
Generated-By: PostHog Desktop
Task-Id: 334f773a-ab58-4546-98cf-39d3f79448cc
@pauldambra
pauldambra force-pushed the posthog/golden-pr-evals branch from b915476 to f6eceeb Compare September 29, 2026 07:14

@posthog posthog Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

PostHog Review

Found 2 should fix.

Comment on lines +150 to +151
result, candidate, agent_log = evaluate(pr, args.runtime, model, args.judge_model, args.case_timeout, args.repo)
write_result(results_dir, result, candidate, agent_log)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

Continue the batch when one case fails

should_fix bug

Issue description

The CLI runs selected PRs in a serial loop, but an exception from evaluate() or write_result() exits the loop. In the default run, one case-specific failure can prevent every later PR from being evaluated.

Why we think it's a valid issue
  • Checked: Read main() and the evaluation and result-writing paths in products/tasks/evals/golden_prs/__main__.py, plus the local-run instructions in products/tasks/evals/golden_prs/README.md.
  • Found: main() calls evaluate() and write_result() directly inside the loop at products/tasks/evals/golden_prs/__main__.py:148-151, with no per-case exception handling. Those operations can raise: for example, ensure_golden_commits() runs a checked Git fetch at products/tasks/evals/golden_prs/workspace.py:64-69, and checkout_parent() raises when archive or extraction fails at products/tasks/evals/golden_prs/workspace.py:135-142.
  • Impact: The README documents running all PRs locally by omitting --pr at products/tasks/evals/golden_prs/README.md:36-38. If one case raises, the process exits before evaluating later cases or printing the final report at products/tasks/evals/golden_prs/__main__.py:153-155. Per-case failure reporting and a non-zero final status would preserve the batch results while still signaling failure.
Suggested fix

Handle failures per PR so the loop can continue, and include each failed case in the final summary with its error. Return a non-zero exit status after the batch if any case failed.

Prompt to fix with AI (copy-paste)
## Context
@products/tasks/evals/golden_prs/__main__.py#L150-151

<issue_description>
The CLI runs selected PRs in a serial loop, but an exception from `evaluate()` or `write_result()` exits the loop. In the default run, one case-specific failure can prevent every later PR from being evaluated.
</issue_description>

<issue_validation>
- **Checked:** Read `main()` and the evaluation and result-writing paths in `products/tasks/evals/golden_prs/__main__.py`, plus the local-run instructions in `products/tasks/evals/golden_prs/README.md`.
- **Found:** `main()` calls `evaluate()` and `write_result()` directly inside the loop at `products/tasks/evals/golden_prs/__main__.py:148-151`, with no per-case exception handling. Those operations can raise: for example, `ensure_golden_commits()` runs a checked Git fetch at `products/tasks/evals/golden_prs/workspace.py:64-69`, and `checkout_parent()` raises when archive or extraction fails at `products/tasks/evals/golden_prs/workspace.py:135-142`.
- **Impact:** The README documents running all PRs locally by omitting `--pr` at `products/tasks/evals/golden_prs/README.md:36-38`. If one case raises, the process exits before evaluating later cases or printing the final report at `products/tasks/evals/golden_prs/__main__.py:153-155`. Per-case failure reporting and a non-zero final status would preserve the batch results while still signaling failure.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Handle failures per PR so the loop can continue, and include each failed case in the final summary with its error. Return a non-zero exit status after the batch if any case failed.
</potential_solution>

${MODEL:+--model "$MODEL"} \
--judge-model "$JUDGE_MODEL" \
--case-timeout "$((10#$CASE_TIMEOUT_MINUTES * 60))" \
--results-dir "$RESULTS_DIR"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FLASH MODE - Faster, but stupid, use regular ReviewHog for a heavy review

[should_fix] Keep agent-written files out of score results

should_fix bug

Issue description

The agent inherits RESULTS_DIR and runs without a filesystem sandbox. It can write a fabricated .json file under this directory while it works. The runner later loads every JSON file recursively from the same directory, so that file can add fake scores to the report and distort the mean.

Why we think it's a valid issue
  • Checked: Traced RESULTS_DIR from .github/workflows/golden-pr-evals.yml through run_agent and the artifact/report steps.
  • Found: The workflow passes RESULTS_DIR to the eval command at .github/workflows/golden-pr-evals.yml:181. agent_environment removes only GitHub credentials and the other provider key, so the agent inherits RESULTS_DIR (products/tasks/evals/golden_prs/agents.py:39-41). The workflow uploads that directory (.github/workflows/golden-pr-evals.yml:183-189), then downloads the artifacts into it and runs the report (.github/workflows/golden-pr-evals.yml:212-229). load_results reads every *.json recursively (products/tasks/evals/golden_prs/__main__.py:88-89), and report includes every loaded result in the rows and means (products/tasks/evals/golden_prs/__main__.py:92-108).
  • Impact: An agent-written JSON file can be included in the uploaded artifacts and loaded by the report job, adding a fabricated score to the table and changing its means. This is a reachable score-integrity bug, so the finding meets the bar.
Suggested fix

Isolate the agent from the evaluator's result directory, then have the report read only the expected result files for the selected PRs. A separate container or user for the agent provides a stronger boundary than hiding the path from its environment.

Prompt to fix with AI (copy-paste)
## Context
@.github/workflows/golden-pr-evals.yml#L181

<issue_description>
The agent inherits `RESULTS_DIR` and runs without a filesystem sandbox. It can write a fabricated `.json` file under this directory while it works. The runner later loads every JSON file recursively from the same directory, so that file can add fake scores to the report and distort the mean.
</issue_description>

<issue_validation>
- **Checked:** Traced `RESULTS_DIR` from `.github/workflows/golden-pr-evals.yml` through `run_agent` and the artifact/report steps.
- **Found:** The workflow passes `RESULTS_DIR` to the eval command at `.github/workflows/golden-pr-evals.yml:181`. `agent_environment` removes only GitHub credentials and the other provider key, so the agent inherits `RESULTS_DIR` (`products/tasks/evals/golden_prs/agents.py:39-41`). The workflow uploads that directory (`.github/workflows/golden-pr-evals.yml:183-189`), then downloads the artifacts into it and runs the report (`.github/workflows/golden-pr-evals.yml:212-229`). `load_results` reads every `*.json` recursively (`products/tasks/evals/golden_prs/__main__.py:88-89`), and `report` includes every loaded result in the rows and means (`products/tasks/evals/golden_prs/__main__.py:92-108`).
- **Impact:** An agent-written JSON file can be included in the uploaded artifacts and loaded by the report job, adding a fabricated score to the table and changing its means. This is a reachable score-integrity bug, so the finding meets the bar.
</issue_validation>

## Task
Investigate the issue and solve it

<potential_solution>
Isolate the agent from the evaluator's result directory, then have the report read only the expected result files for the selected PRs. A separate container or user for the agent provides a stronger boundary than hiding the path from its environment.
</potential_solution>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

feature/desktop Feature Tag: Desktop

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant